BREAKING CHANGE: CertReq: Improve certificate match selectivity #271

uw-dc · 2022-12-14T17:13:30Z

Pull Request (PR) description

Breaking Change: Support multiple certificates with the same issuer and subject by making friendlyName a mandatory (key) parameter

Prevent existing certificates with non-matching friendlyName and template being considered as existing desired certificates; This resolves problems where an incorrect existing certificate is compared with the desired state and Test-TargetResource consequentially returns $false.

This Pull Request (PR) fixes the following issues

Task list

Added an entry to the change log under the Unreleased section of the
file CHANGELOG.md. Entry should say what was changed and how that
affects users (if applicable), and reference the issue being resolved
(if applicable).
Resource documentation added/updated in README.md.
Resource parameter descriptions added/updated in README.md, 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 Community Testing Guidelines.
Integration tests added/updated (where possible). See DSC Community Testing Guidelines.
New/changed code adheres to DSC Community Style Guidelines.

This change is

…o FriendlyName

…tched on Subject and Issuer

…fault template !

…te matching desired certificate template

Many situations require multiple certificates, for example DSC Encryption + private web server breaking change

uw-dc · 2022-12-14T17:22:32Z

An issue with 'GENERATE CONCEPTUAL HELP'; Looks environmental?

ERROR: Failed to import classes from file /home/vsts/work/1/s/source/DSCResources/DSC_CertificateExport/DSC_CertificateExport.schema.mof. Error Exception calling "ImportClasses" with "3" argument(s): "Unable to load shared library 'libmi' or one of its dependencies. In order to help diagnose loading problems, consider setting the LD_DEBUG environment variable: liblibmi: cannot open shared object file: No such file or directory"
At /home/vsts/work/1/s/output/RequiredModules/DscResource.DocGenerator/0.11.1/DscResource.DocGenerator.psm1:1486 char:9
+         throw "Failed to import classes from file $FileName. Error $_ …
+         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

"Unable to load shared library 'libmi' or one of its dependencies

`"Unable to load shared library 'libmi' or one of its dependencies` Hoping that PSWSMan will satisfy dependency

`"Unable to load shared library 'libmi' or one of its dependencies` This won't be a problem using a Windows image, as per many of the other recently maintained DSC community modules

codecov · 2022-12-15T11:11:24Z

Codecov Report

Merging #271 (f94fb00) into main (137a69f) will increase coverage by 0%.
The diff coverage is 100%.

@@         Coverage Diff         @@
##           main   #271   +/-   ##
===================================
  Coverage    84%    84%           
===================================
  Files         7      7           
  Lines       929    933    +4     
===================================
+ Hits        786    790    +4     
  Misses      143    143

Impacted Files	Coverage Δ
source/DSCResources/DSC_CertReq/DSC_CertReq.psm1	`100% <100%> (ø)`

…te FriendlyName comparison occurs This is when Test-TargetResource is run with CertificateTemplate == 'DomainControllerAuthentication' I would expect a DomainController to only have one maintained certificate used for DomainControllerAuthentication, rather than have multiple with distinct friendly names, so it seems fair to maintain this exception and include a specific test for this scenario

When there were other non-SAN certificate extensions, the situation was treated as Current and Desired SANs do not match rather than there being no SANs

uw-dc · 2022-12-15T14:03:09Z

@PlagueHO

Is it possible please to get a review on this?
It would be really good for us to resolve the issues this PR covers as issue #121 is affecting our production hosts.

Thank you.

PlagueHO

Hi @uw-dc - sorry about the delay. Looks great. Just some minor tweaks, nothing major.

Reviewed 2 of 5 files at r1, 1 of 1 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @uw-dc)

azure-pipelines.yml line 27 at r3 (raw file):

        displayName: 'Package Module'
        pool:
          vmImage: 'windows-latest'

We typically use ubuntu-latest for the package step. Not that it matters too much - but if there is a specific need then we should note it here (so I don't revert it later)

CHANGELOG.md line 8 at r3 (raw file):

## [Unreleased]

- CertReq:

Can you move under the ### Changed section`?

CHANGELOG.md line 9 at r3 (raw file):

- CertReq:
  - Made Certificate FriendlyName a mandatory parameter (key) so multiple certificates with same issuer and subject can be configured in the same DSC configuration - Fixes [Issue #269](https://github.com/dsccommunity/CertificateDsc/issues/269)

Can you reduce the line length to less than 100 chars on this line and the next one?

CHANGELOG.md line 9 at r3 (raw file):

- CertReq:
  - Made Certificate FriendlyName a mandatory parameter (key) so multiple certificates with same issuer and subject can be configured in the same DSC configuration - Fixes [Issue #269](https://github.com/dsccommunity/CertificateDsc/issues/269)

Can you include BREAKING CHANGE: at the beginning of these lines? This will tell the pipeline to increase the major version number.

source/DSCResources/DSC_CertReq/DSC_CertReq.psm1 line 202 at r3 (raw file):

    $cert = Get-ChildItem -Path Cert:\LocalMachine\My |
        Where-Object -FilterScript {
            (Compare-CertificateSubject -ReferenceSubject $_.Subject -DifferenceSubject $Subject) -and `

FYI: I like the improvement here - nice!

source/DSCResources/DSC_CertReq/en-US/DSC_CertReq.strings.psd1 line 17 at r3 (raw file):

    NoValidCertificateMessage = No valid certificate found with Subject '{0}', Issuer '{1}', FriendlyName '{2}' and CertificateTemplate '{3}'.
    ExpiredCertificateMessage = The certificate found with Subject '{0}', Issuer '{1}', FriendlyName '{2}' and CertificateTemplate '{3}' has expired: {4}.
    NoExistingSans = The certificate found with Subject '{0}', Issuer '{1}', FriendlyName '{2}' and CertificateTemplate '{3}' has no SANs, yet the following SANs are specified: {4}. Certificate has the Thumbprint '{5}'.

FYI: Consistency - Can you add Message to the end of the property name to differentiate it? Unless it is an error then add "Error" - not a major thing as we clearly haven't been 100% consistent here.

tests/Unit/DSC_CertReq.Tests.ps1 line 1874 at r3 (raw file):

                    -MockWith $mock_getCertificateTemplateName_validCertificateTemplate

                # Mock -CommandName Get-CertificateSubjectAlternativeName `

Can you remove commented out lines here (and throughout)?

tests/Unit/DSC_CertReq.Tests.ps1 line 1878 at r3 (raw file):

                It 'Should return true' {
                    Test-TargetResource @paramsStandard -Verbose | Should -Be $true

Can you change to Should -BeTrue (and Should -BeFalse) and throughout.

…on Linux The WSMan client libraries libmi.so/libmi.dylib were from the microsoft/omi repository. The OMI project is in maintenance mode and its team has decided to not support PowerShell anymore

PlagueHO · 2022-12-18T20:34:23Z

Sorry @uw-dc - will get onto this tonight.

PlagueHO

Reviewed 5 of 5 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @uw-dc)

CHANGELOG.md line 9 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you reduce the line length to less than 100 chars on this line and the next one?

Thank you! You can also wrap if you want. For example:

 - Made Certificate FriendlyName a mandatory parameter (key) so multiple
   certificates with same issuer and subject can be configured in the same
   DSC configuration - Fixes [Issue #269](https://github.com/dsccommunity/CertificateDsc/issues/269)

PlagueHO

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @uw-dc)

PlagueHO

Thank you for contributing this! We'll leave it in prerelease for a few weeks and then once we'll push it out as a new major version.

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @uw-dc)

…ommunity#271)

uw-dc added 22 commits December 13, 2022 17:29

Get with 2 certs matched on Subject and Issuer

4b599f4

white space

7d6044b

Get with 2 certs matched on Subject and Issuer - undesired cert has n…

8029875

…o FriendlyName

Move feature specific mocks into feature

9ba00a6

Test-TargetResource tests for scenarios with multiple certificates ma…

23b836c

…tched on Subject and Issuer

Get and Test for multi-valid cert scenarios with different templates

e5868cc

Mock certificate template extension more accurately

23a05e1

superfluous characters

28d67b9

to test with non-defaulted template we params need to select a non-de…

c782099

…fault template !

use defaulted value (same)

9874816

use appropriate set of params

a84cbd9

debugging multiple certs difficult when thumbprints are the same

e038ca1

Include matches on Cert Friendly Name

e5adf4a

Further refine matches to include only those certificates with templa…

1de0a2d

…te matching desired certificate template

we can use Get-CertificateTemplateName

e9e63c2

Another case of using wrong parameter set

06cb559

Use matched Template and Friendly Name in test

aa824bc

Issuer and common name are not enough to guarantee uniquness

257e9d4

Many situations require multiple certificates, for example DSC Encryption + private web server breaking change

revert

8e957c8

cleanup

231127a

Update CHANGELOG

ffcaccb

Include FriendlyName and CertificateTemplate in verbose messages

68cc4aa

uw-dc added 3 commits December 15, 2022 10:12

'GENERATE CONCEPTUAL HELP' pipeline build step results in:

3c28c36

`"Unable to load shared library 'libmi' or one of its dependencies` Hoping that PSWSMan will satisfy dependency

revert 3c28c36

fe90aec

'GENERATE CONCEPTUAL HELP' pipeline build step results in:

a7a761e

`"Unable to load shared library 'libmi' or one of its dependencies` This won't be a problem using a Windows image, as per many of the other recently maintained DSC community modules

uw-dc added 3 commits December 15, 2022 11:18

Certificates with non-matching template name will no longer be selected

76627f0

Redundant localized string

b8d7e61

uw-dc added 4 commits December 15, 2022 12:17

white space

7108e03

Refactor removes orphan code

9e501ac

When there were other non-SAN certificate extensions, the situation was treated as Current and Desired SANs do not match rather than there being no SANs

Should have verbose logging with SAN mismatches

ef730fe

punctutation in verbose messages

c68376f

johlju requested a review from PlagueHO December 15, 2022 15:10

PlagueHO requested changes Dec 15, 2022

View reviewed changes

uw-dc added 7 commits December 15, 2022 23:04

libmi is no longer included on account WS-MAN is no longer supported …

d5af1fb

…on Linux The WSMan client libraries libmi.so/libmi.dylib were from the microsoft/omi repository. The OMI project is in maintenance mode and its team has decided to not support PowerShell anymore

CHANGELOG.md entries must be less than 100 characters

1e81723

indicate breaking change

a74c25a

localized string data keys should be suffixed with message type

7348ecd

Remove commented out code

befac12

preferred syntax

5419b0a

Correct localized data keys used in tests

f94fb00

uw-dc requested a review from PlagueHO December 16, 2022 09:18

PlagueHO requested changes Dec 23, 2022

View reviewed changes

PlagueHO approved these changes Dec 23, 2022

View reviewed changes

PlagueHO merged commit 8d5f01f into dsccommunity:main Dec 23, 2022

PlagueHO mentioned this pull request Dec 23, 2022

CertReq: Add FriendlyName comparison #206

Closed

9 tasks

uw-dc deleted the improve-certreq-certificate-match-selectivity branch December 28, 2022 09:08

cmielke pushed a commit to cmielke/CertificateDsc that referenced this pull request Jul 3, 2023

BREAKING CHANGE: CertReq: Improve certificate match selectivity (dscc…

33329ff

…ommunity#271)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BREAKING CHANGE: CertReq: Improve certificate match selectivity #271

BREAKING CHANGE: CertReq: Improve certificate match selectivity #271

uw-dc commented Dec 14, 2022 •

edited

Loading

uw-dc commented Dec 14, 2022 •

edited

Loading

codecov bot commented Dec 15, 2022 •

edited

Loading

uw-dc commented Dec 15, 2022

PlagueHO left a comment

PlagueHO commented Dec 18, 2022

PlagueHO left a comment

PlagueHO left a comment

PlagueHO left a comment

BREAKING CHANGE: CertReq: Improve certificate match selectivity #271

BREAKING CHANGE: CertReq: Improve certificate match selectivity #271

Conversation

uw-dc commented Dec 14, 2022 • edited Loading

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

Task list

uw-dc commented Dec 14, 2022 • edited Loading

codecov bot commented Dec 15, 2022 • edited Loading

Codecov Report

uw-dc commented Dec 15, 2022

PlagueHO left a comment

Choose a reason for hiding this comment

PlagueHO commented Dec 18, 2022

PlagueHO left a comment

Choose a reason for hiding this comment

PlagueHO left a comment

Choose a reason for hiding this comment

PlagueHO left a comment

Choose a reason for hiding this comment

uw-dc commented Dec 14, 2022 •

edited

Loading

uw-dc commented Dec 14, 2022 •

edited

Loading

codecov bot commented Dec 15, 2022 •

edited

Loading