search

d365collaborative / d365fo.tools

Tools used for Dynamics 365 Finance and Operations
MIT License
246 stars 101 forks source link

🐛 fix error when running Enable-D365MaintenanceMode not as administrator #590

Closed FH-Inway closed 2 years ago

FH-Inway commented 2 years ago

When running the Enable-D365MaintenanceMode cmdlet from a PowerShell session without administrator privileges, an error is shown and the maintenance mode is not enabled.

Repro steps

  1. Open PowerShell without administrator privileges
  2. Run Enable-D365MaintenanceMode

Result Error message 

[Get-SQLCommand] Something went wrong while working with the sql server connection objects | Exception setting "ConnectionString": "The value's length for key 'password' exceeds it's limit of '128'."

See Gist d365fo.tools Enable-D365MaintenanceMode error for the full error message.

Expected result Maintenance mode is enabled without error message, just like when running the cmdlet as administrator. Or if that is not possible, a warning message should inform the user that enabling the maintenance mode requires administrator privileges.

FH-Inway commented 2 years ago

The same issue occurs when Disable-D365MaintenanceMode is executed without administrator privileges.

FH-Inway commented 2 years ago

I also tried cmdlet Get-D365User thinking that the issue might impact all cmdlets that use a SQL script. However, that cmdlet was able to execute without administrator privileges. I think it might be related to how the parameters for the Get-SqlCommand call are created.

Splaxi commented 2 years ago

I'll do some testing tonight 👍

Splaxi commented 2 years ago

The 128 char limit / exception is normally tied back to the capabilities to descrypt the web.config file.

Why one cmdlet is capable of doing just that, while another isn't - seems odd. As promised, I'll dive into the details tonight.

Splaxi commented 2 years ago

Okay - things are a bit complicated.

I have been testing on a CHE - my VPN isn't working, so I can't test against an onebox. But my memory should serve me right on this one:

It all boils down to what privileges Microsoft has enforced on the different components / areas / layers on either the CHE build or the Onebox build.

Getting the webconfig file is easy - but decrypting it is the hard part. While running in elevated (RunAs Administrator), our powershell session is capable of opening the certificate store and using the needed certificate to decrypt the web.config on the fly and obtain the sql password.

This explains the Exception setting "ConnectionString": "The value's length for key 'password' exceeds it's limit of '128' - generally speaking.

What you found, is in fact a bug: Get-D365User (Works) Line 80: We test whether or not we can use a Trusted Connection (AKA - Windows Authentication, current user signed in) Line 86: We pass the object (true in this case) directly to the command builder and get a SQL connection object that will use Trusted Connection = True (AKA - NO SQL USER OR PASSWORD) https://github.com/d365collaborative/d365fo.tools/blob/91be942e9ed19e2c84133c4058a78aa950f9e14f/d365fo.tools/functions/get-d365user.ps1#L80-L86

Enable-D365MaintenanceMode Line 117: We test whether or not we can use a Trusted Connection (AKA - Windows Authentication, current user signed in) Line 132: We pass the object (true in this case) into a SUB function, Invoke-D365SqlScript... https://github.com/d365collaborative/d365fo.tools/blob/91be942e9ed19e2c84133c4058a78aa950f9e14f/d365fo.tools/functions/enable-d365maintenancemode.ps1#L117-L132

Invoke-D365SqlScript Remember line 119-125 from Enable-D365MaintenanceMode?? Line 82: We test whether or not we can use a Trusted Connection (We already did test it), but because we supplied SqlUser and SqlPwd, the second Test-TrustedConnection disables the Trusted Connection. https://github.com/d365collaborative/d365fo.tools/blob/91be942e9ed19e2c84133c4058a78aa950f9e14f/d365fo.tools/functions/invoke-d365sqlscript.ps1#L63-L82

I believe the fix is to move the first elseif (2nd logical testing) down one and move the second elseif (3rd logical testing) up and that way always test for TrustedConnection first before going for SqlUser / SqlPwd. https://github.com/d365collaborative/d365fo.tools/blob/91be942e9ed19e2c84133c4058a78aa950f9e14f/d365fo.tools/internal/functions/test-trustedconnection.ps1#L28-L43

But changing this could be breaking change, as this logic has been around for so long and people most like are used to the way it works now. On the other side, looking at the current cmdlets that double jump like this are:

Finally - Do we as a community gain anything while supporting non-Administrator / non-Elevated? All CHE gives us access to the Administrator. All PowerShell sessions started by that account, auto-elevates. Onebox is a different breed entirely. Here people needs to learn how to configure the PowerShell session to always elevate (which will prompt them) - but when that is first in place, things are easier for everyone and will behave the same way.

FH-Inway commented 2 years ago

Nice analysis and I like how the links to line numbers in a file are displayed.

I guess the question on supporting non-Administrator should at least be put into a discussion to get more eyes on it. I created #596 for that.

For cmdlets that currently fail in non-Administrator execution, I suggest we add a warning that informs the user that the cmdlet can only be executed with admin privileges and prevent the error message.

For other cmdlets, maybe add a similar warning but without preventing the execution?

Does PowerShell have a hook when a module is loaded/imported, so we could check then for admin privilege?

Splaxi commented 2 years ago

So PowerShell as a concept doesn't have what we are looking for - but we implemented our own solution.

$Script:IsAdminRuntime https://github.com/d365collaborative/d365fo.tools/blob/c34e0fd70311f7b368c618f0a54c29a917428d53/d365fo.tools/functions/set-d365sdpcleanup.ps1#L35-L39

https://github.com/d365collaborative/d365fo.tools/blob/c9fe5fcf8608fdb224986bbddc1f630a2f601729/d365fo.tools/internal/scripts/variables.ps1#L18

It is based on the assumption, that an user in CHE or onebox, will be part of the built-in Administrators User Group on the Windows Installation. It was perfect back in the days - and I expect it to still be working as intended.

FH-Inway commented 2 years ago

Looks good, maybe extract it into an external function so there is a consistent behavior across all cmdlets? Probably with a parameter that determines if the calling function should be stopped.

FH-Inway commented 2 years ago

So something like https://github.com/FH-Inway/d365fo.tools/blob/%23590-CheckForAdminRuntime/d365fo.tools/internal/functions/test-isadminruntime.ps1#L27-L42

And then used like https://github.com/FH-Inway/d365fo.tools/blob/%23590-CheckForAdminRuntime/d365fo.tools/functions/enable-d365maintenancemode.ps1#L104-L108

User experience would look like this:

PS C:\windows\system32> Enable-D365MaintenanceMode
[19:33:53][Test-IsAdminRuntime] It seems that you ran this cmdlet non-elevated. To run this cmdlet, it needs to be called from an elevated console. Please exit the current console and start a new one with "Run As Administrator"
[19:33:53][Test-IsAdminRuntime] Elevated permissions needed. Please start an elevated session and run the cmdlet again.
[19:33:53][Enable-D365MaintenanceMode] Maintenance mode can only be enabled in an elevated session.
WARNING: [19:33:53][Enable-D365MaintenanceMode] Stopping because of running non-elevated.

What do you think?

As an aside, while writing this, I went down a bit of a rabbit hole: I first tried to do this with Stop-PSFFunction and along the way came across #265, which discusses how exception should be handled by the module. I also came across #173 Test-PSFFunctionInterrupt doesn't catch Stop-PSFFunction, which is why I decided to not use it.

Splaxi commented 2 years ago

I did read all - but will just comment on the rabbit hole for now:

We actually solved that together with Fred.

https://github.com/d365collaborative/d365fo.tools/blob/c34e0fd70311f7b368c618f0a54c29a917428d53/d365fo.tools/internal/functions/test-pathexists.ps1#L81-L85

Note the -StepsUpward 1 parameter ? This marks the callstack, so that the parent caller function can rely on Test-PSFFunctionInterrupt.

This is in place for all cmdlets that use the Test-PathExists.

So your idea is solid, you just need this small trick and then you're good to go 🤘

FH-Inway commented 2 years ago

Got it. Could you take another look at the two changes?

Splaxi commented 2 years ago

Coming with 0.6.70 in 20 minutes