Skip to content
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

Certificate multi-matcher #95

Closed

Conversation

lamawithonel
Copy link

@lamawithonel lamawithonel commented Oct 4, 2021

Pull Request (PR) description

This PR makes it possible to set multiple values for Issuer and DN (makes them arrays). Setting them as arrays allows a single configuration be used for a wider range of circumstances, such as multi-cloud environments where the issuer might be different in each. To achieve this the Find-Certificate function has been overhauled from a series of if-then-else statements, to something more like a compiler for a Where-Object filter script block. This reduces repetition and complexity, and should make it easier to add additional filter parameters. In cases where multiple matching certificates exist, the final list is sorted based on the array input order to ensure determinism, and the first one is returned.

Care has been taken to ensure all existing tests pass, although some minor modifications were required to add new tests. Several additional tests were added.

This PR also fixes #89 as a separate commit, although those changes have no functional effect.

Additional information in the commit comments.

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 Reviewable

In X.500 parlance, a _Distinguished Name_ (_DN_) is a fully-qualified
object path<sup>[1][1],[2][2]</sup>  For example, `CN=example.com,
O=IANA, C=US` is a DN.  Prior to this, however, the `DN` parameter did
not specify the fully-qualified DN, but instead only the path part,
minus the _Common Name_.  In the above example, that would be `O=IANA,
C=US`, were the common name is `CN=example.com`.  This path part is
properly known as the _Base DN_.<sup>[2][2],[3][3]</sup>  Therefore,
this commit updates the documentation for the `DN` parameter with the
appropriate names.

Fixes dsccommunity#89

[1]: https://ldapwiki.com/wiki/Distinguished%20Names
[2]: https://www.novell.com/documentation/extend5/Docs/help/Composer/books/LDAPGlossary.html
[3]: https://ldapwiki.com/wiki/BaseDN
Prior to this the tests for `Find-Certificate` only ever tested empty or
single-element CertStore mocks.  The documented behavior when
`SubjectFormat == Both` is to select the FQDN cert over the short name
cert, which requires a CertStore with two or more certificates.  This
commit modifies the mocks and adds a test for that condition.

This change endeavored not to change any of the existing tests, but due
to some changes in the mocks, a few minor changes were required.
Namely, the mock call counts for a few tests changed, because the
default certificate now uses the short hostname rather than the FQDN.
Absolutely non of the module code has changed, however, so the pure
functionality of the DSC resources remains intact.
Prior to this the `Find-Certificate` function was inconsistent about
checking for the `serverAuthentication` attribute, a requirement for
HTTPS servers such as WSMan listners.  This commit fixes this and takes
some inital steps to remove duplicate code that might lead to similar
oversights in the future.  To do this, it moves the
`serverAuthentication` filter from the `Where-Object` script block to a
certificate provider mixin for `Get-ChildItem`-- the
`-SSLServerAuthentication` parameter.  To reduce duplication of code,
it also moves the function's `Get-ChildItem` parameters, which are
always the same, into a private `PSDefaultParameters` block.
Prior to this the `Find-Certificate` function in `WSManListner` used a
complex web of `if-then-else` statements to try successive combinatorial
conditions.  Adding additional features in that framework was daunting,
because the combinatorial explosion would require an expodential
addition of conditional statmenets.  Namely, I wanted to make it
possible to pass in multiple values for `Issuers` and/or `DN` to match
the variety of CA certificates I deal with.  Rather than adding more to
the pile, this commit refactors-- essentially rewrites-- the
`Find-Certificate` function to work more like a rudimentary compiler.
As a nice side-effect, I was able to add the multi-matcher feature.

The implementation starts with a `Hashtable` (`$X509Properites`) to
represent the filter criteria.  Each key in the hashtable represents a
potential property of a certificate, and the values represent possible
property values.  Once the `Hashtable` is set, this new implementation
compiles it into expressions, and joins them together using `-and`.  The
final expression string is then cast into a `ScriptBlock` and passed to
`Where-Object -FilterScript`.

A few tests are added to demonstrate the new multi-matcher feature.  The
mock call counters on all `Find-Certificate` tests have also been
updated, because `Get-ChildItem` is now only ever called once.

I've done my best to ensure this doesn't break any of the current tests
or documented functionality, but it is possible there are some edge
cases where this will break things.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WSManListener parameter DN should be BaseDN
1 participant