New-DbaComputerCertificate, options for Algorithm and Period validity

[daniel-merchant]

Please read -- recent changes to our repo

On November 10, 2022, we removed some bloat from our repository (for the second and final time). This change requires that all contributors reclone or refork their repo.

PRs from repos that have not been recently reforked or recloned will be closed and @potatoqualitee will cherry-pick your commits and open a new PR with your changes.

• [?] Please confirm you have the smaller repo (85MB .git directory vs 275MB or 110MB or 185MB .git directory) -Apologies I really do not know how I would check this. I forked the latest development branch.

Type of Change

• [ ] Bug fix (non-breaking change, fixes # )
• [x] New feature (non-breaking change, adds functionality, fixes #9263)
• [ ] Breaking change (affects multiple commands or functionality, fixes # )
• [x] Ran manual Pester test and has passed (.\tests\manual.pester.ps1)
• [ ] Adding code coverage to existing functionality
• [x] Pester test is included
• [ ] If new file reference added for test, has is been added to github.com/dataplat/appveyor-lab ?
• [x] Unit test is included
• [ ] Documentation
• [ ] Build system

Purpose

Added Hashing Algorithm and Exipiry date options to New-DbaComputerCertificate

Approach

Added two variables to the New-DbaComputerCertificate function "HashingAlgorithm" and "MonthsValid" Expanded tests to ensure default values are kept and that the new optional variables are respected.

Commands to test

New-DbaComputerCertificate

Learning

Apologies, I am not very familiar with GIT and not a regular contributor to DBATools. I have read the contributing guidelines and I hope this will be useful, but please let me know if this is not appropriate or needs some work and I am happy to take on any criticism.

Inspired as we are going to be implementing something something that is likely going to need these options. This might be a popular request following this blog article: https://www.brentozar.com/archive/2024/03/read-this-before-your-users-install-ssms-20/

[daniel-merchant]

Should resolve story I opened #9263

[niphlod]

hi @daniel-merchant , everything looks fine. Is the test specifically disabled in appveyor for some specific reason ?

[niphlod]

BTW, you miss a run of Invoke-DbatoolsFormatter for this file, or you can manually indent correctly lines 356-357 . Also, remove bin/dbatools-index.json from the list of changed files

[daniel-merchant]

BTW, you miss a run of Invoke-DbatoolsFormatter for this file, or you can manually indent correctly lines 356-357 . Also, remove bin/dbatools-index.json from the list of changed files

Sorry about that. I will run the formatter and update (probably tomorrow now). I will also remove that file from the changed list.

hi @daniel-merchant , everything looks fine. Is the test specifically disabled in appveyor for some specific reason ? Hi @niphlod. This is not something I have done intentionally. That was how the existing test looked, so I copied it. Happy to remove the "if (-not $env:appveyor)" statement around the tests if that is preferred?

Apologies, this is one of my first commits to any public repository. Thank you for you patience.

[niphlod]

No worries, it's pratically perfect and there's no rush .

Regarding tests... Theoretically if you remove the if (-not $env:appveyor) from the test it will run each time on the countinous integration infrastructure so we can be sure no regressions will be introduced in a later stage. There are some things that can't run in appveyor, and I (really) don't know if creating a self-signed cert can be done or not. The point here is that we should try to make all possible things run on appveyor if they work so sudden bugs can't be introduced into the codebase. If for some specific reason it does not, we can disable the testing on appveyor but "leave a note" so everyone can know the reason.

[niphlod]

it fails with "Cannot bind argument to parameter 'Thumbprint' because it is null." so probably it was explicitely disabled because certificate generation does not work on appveyor, bummer. re-add the if (-not $env:appveyor) and let's call it a day.

Last minor fix, please keep EnabledException as the last param

[daniel-merchant]

@niphlod - I have moved EnableException to the last parameter and re-enabled the appveyor exclusions.

Please do let me know if there is anything else I need to do here. Really appreciate the work that is put into these community tools.

[niphlod]

ok @daniel-merchant , failures in appveyor are unrelated

[potatoqualitee]

Thank you both 🙇🏼 Will merge when tests approve

[andreasjordan]

Before this change, the command worked on my lab with a german locale. Now it does not because of the date format of NotBefore and NotAfter. I invested some minutes but have not found a solution.

Does anyone have experienced the same problem?

[niphlod]

@andreasjordan , it very well may be but till we use certreq, we are in the realm of creating an .inf, whose specs just say

""" Tip: NotBefore and NotAfter are for RequestType=cert only. Date parsing attempts to be locale-sensitive. Using month names will disambiguate and should work in every locale. """

[andreasjordan]

I had no luck with month names. Tried a lot of date formats - nothing works.

[niphlod]

then it's more of a certreq.exe problem to solve before understanding how to do it within dbatools code. Either that, or we use ValidityPeriod and ValidityUnits, assuming not specifying a start date defaults to "now".

[andreasjordan]

Does not work. Probably because of english names of periods.

Too bad. We have to find a different way for the german systems.

[niphlod]

or we set the thread to english just before invoking certreq ??

[andreasjordan]

Maybe we are looking at the wrong point.

Have a look at the specs you linked:

Tip: NotBefore and NotAfter are for RequestType=cert only. Date parsing attempts to be locale-sensitive. Using month names will disambiguate and should work in every locale.

But "RequestType=cert" is only used to self-signing certs. For the others, we use "RequestType = PKCS10", where these parameters are not allowed. So it's not the language but the parameter itself.

I can successfully create a self-signed certificate with that command, but not a "normal" certificate issued by my lab-CA.

[andreasjordan]

The documentation is correct: "Allows you to specify the number of months a self-signed certificate will be valid for"

So we only need to change the code to put the two lines in the if block. Will open a pull request for that.

[daniel-merchant]

@andreasjordan sorry my changes broke this scenario. Should have checked the documentation better for those additional parameters. Thank you for fixing.