SqlServerDatabaseMail: Added Authentication configuration

[t3mi]

Pull Request (PR) description

Changes to SqlServerDatabaseMail:

• Added new parameter EnableSsl which controls encryption of communication using Secure Sockets Layer.
• Added new parameter Authentication and SMTPAccount for configuration of SMTP authentication mode and credential used.
• Added new helper function Get-MailServerCredentialId which gets credential Id used by mail server.
• Added new helper function Get-ServiceMasterKey which gets unencrypted Service Master Key for specified SQL Instance.
• Added new helper function Get-SqlPSCredential which decrypts and returns PSCredential object of SQL Credential by Id.
• Added DAC switch to function 'Connect-SQL' with default value $false. Used to specify that non-pooled Dedicated Admin Connection should be established.
• Added UsingDAC switch to function 'Invoke-Query' with default value $false. Used to specify that query should be executed using Dedicated Admin Connection. After execution DAC connection will be closed.
• Added PSCredential option to function 'Test-DscParameterState' which will compare PSCredential objects using username/password keys.

Submission containing materials of a third party: Antti Rantasaari 2014, NetSPI License: BSD 3-Clause https://opensource.org/licenses/BSD-3-Clause Source: https://github.com/NetSPI/Powershell-Modules/blob/master/Get-MSSQLCredentialPasswords.psm1

This Pull Request (PR) fixes the following issues

Fixes issue #1215

Task list

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

---

This change is 

[codecov-io]

Codecov Report

Attention: Patch coverage is 95.67901% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 98%. Comparing base (728a1cf) to head (267b931). Report is 331 commits behind head on main.

Additional details and impacted files

@@          Coverage Diff          @@
##           main   #1374    +/-   ##
=====================================
  Coverage    98%     98%            
=====================================
  Files        36      36            
  Lines      5359    5515   +156     
=====================================
+ Hits       5253    5406   +153     
- Misses      106     109     +3     

[johlju]

@t3mi Awesome work on this, I will review in a day or so.

[johlju]

@t3mi Impressive work, clean coding! 😃

[t3mi]

@johlju thanks for reviewing this mess :) I'll try to fix in a day or so.

[johlju]

I has thoughts about if we actually need to verify and enforce that the password is correct. It’s a lot of code just to be able to read the password, and the need to get an ADMIN: connection to do it.

Could we just skip and enforce the password? What is your thoughts about that?

[t3mi]

I would leave the check for password. For my use case we have single username and api tokens as passwords and having the password check is crucial if we face expired/modified password or if it was initially set incorrectly we just put the right password instead of spending time to recreate mail account. I understand that possibility of this is low but anyway it's present and if we know how to implement it in the right way - why not? :) Some other parts of the SQL are also using credentials and with this code it would be easy to have desired functionality implemented in those areas as well. I don't like admin connection either but it's the only way to obtain encrypted information. And as you checked I'm not using pooled connection and disconnecting right after getting the results so there would not be any idle/sleeping connection left which will block any subsequent admin connections.

[johlju]

Thank you for that use case. Let’s leave the check as-is then :)

You mention closing the admin-connection, I wonder if we should add that to another helper function so that other contributors close the connection the same way.

It might also be easier for me to remember that using DAC should also use the close-function. :smile:

[johlju]

As-is = after the review comments been resolved I mean :)

[t3mi]

You mention closing the admin-connection, I wonder if we should add that to another helper function so that other contributors close the connection the same way.

It won't work the same way in current state. Right now all connections to SQL are done using connection pool so if you run Disconnect(), connection will not be closed right away but will change its status to sleeping until new request comes in. If you want to have disposable connection than it needs to be non-pooled. In that case Disconnect() will close connection immediately. This was done for DAC connection as there is a limitation of only one DAC connection is supported.

[johlju]

This was done for DAC connection as there is a limitation of only one DAC connection is supported.

I meant for other contributors calling for a DAC connection in the future. That there are a Disconnect-DACConnection (or whatever is a suitable name). But that can be added in another PR.

[t3mi]

Still digging.. I can not get 100% reproduction yet as they sometimes fail sometimes pass with the same incoming parameters. It would be better to not merge this until the issue will be found.

[johlju]

This just merged PR mentioned Connect-SQL having an issue, maybe it is related? 🤔 https://github.com/PowerShell/SqlServerDsc/pull/1376#issuecomment-505283659

[johlju]

The failing test only seem to happen with this PR. 🤔 I created 5 branches (based on dev) and pushed them to my fork and they all passed the tests. Might still me a coincident, but this PR failed twice in a row. Any luck narrowing down the cause?

[t3mi]

Haven't had much time to spend on this. I'm planning to take a look this weekend.

[Rahul05072904]

Can anyone help me to extract the API for misterhouse-pc5401

[johlju]

@Rahul05072904 I think you are in the wrong repository for that question. This repo is resources for Desired State Configuration (DSC). 🙂

[johlju]

The new CI pipeline has merged. This changed the folder structure, and also removed the dev branch. Please rebase against the master branch, and make sure to get your changes into the the file in the new location (under source folder). 😃

Read more here about the new coding workflow: https://dsccommunity.org/guidelines/contributing/#understand-the-coding-workflow https://dsccommunity.org/guidelines/testing-guidelines/#running-tests You can also use Invoke-Pester once you run build.ps1 -Task build (not documented yet) https://dsccommunity.org/guidelines/contributing/#attach-your-fork-to-a-free-azure-devops-organization

[donuwm]

Hey was just wondering about the status on this. Thanks!

[johlju]

I will have a look at this and get it over the finish line as soon as I have time.

[t3mi]

The code should be migrated away from using smo for DAC as it spams errors during config checks into sql server log.

[johlju]

@t3mi You mean removing DAC altogether, or use it in another way? If the latter, do you have a solution that you can share?

[stale[bot]]

Labeling this pull request (PR) as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on this PR is taken up again.

[github-actions[bot]]

Labeling this pull request (PR) as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on this PR is taken up again.

[github-actions[bot]]

Labeling this pull request (PR) as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on this PR is taken up again.

[RandyInMarin]

So, how does this get fixed?

[johlju]

Suggest opening a new PR to continue to work.

[RandyInMarin]

Suggest opening a new PR to continue to work.

I only need one more value - UseDefaultCredentials. Abandon the work already done and just address that?

[johlju]

Would like to see public commands that handle MailAccount (e.g. Get-/Add-/Remove-/Set-SqlDscMailAccount) and mailserver (e.g.g Get-/Add-/Remove-/Set-SqlDscMailServer) where the later commands takes the output from Get-SqlDscMailAccount through the pipeline. Then we can extend the DSC resource more easily I think. But please open a separate issue and describe how you suggest to address UseDefaultCredentials.

[RandyInMarin]

I have forked the repo and made the required updates to DSC_SqlDatabaseMail and the unit tests for UseDefaultCredentials. Running the unit tests now.... (The unit tests work and pointed out problems... fixing my errors now.) First fork. First module build. First use of unit tests. I'm a bit amazed I made it this far. Looking forward to my first pull request... exciting.

[RandyInMarin]

Fixing a unit test is a lot harder than updating the resource.