Certificate multi-matcher #95
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request (PR) description
This PR makes it possible to set multiple values for
IssuerandDN(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 theFind-Certificatefunction has been overhauled from a series ofif-then-elsestatements, to something more like a compiler for aWhere-Objectfilter 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
file CHANGELOG.md. Entry should say what was changed and how that
affects users (if applicable), and reference the issue being resolved
(if applicable).
and comment-based help.
This change is