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

Set-ExecutionPolicy reports statement-terminating error instead of warning #12032

Closed
mklement0 opened this issue Mar 5, 2020 · 13 comments
Closed
Labels
Breaking-Change breaking change that may affect users Committee-Reviewed PS-Committee has reviewed this and made a decision Issue-Question ideally support can be provided via other mechanisms, but sometimes folks do open an issue to get a Resolution-Answered The question is answered. Resolution-By Design The reported behavior is by design. WG-Security security related areas such as JEA

Comments

@mklement0
Copy link
Contributor

mklement0 commented Mar 5, 2020

If you're trying to set a persistent execution policy (-Scope LocalMachine or -Scope CurrentUser) while a process-level policy happens to be in effect (-Scope Process or, more likely, -ExecutionPolicy via the CLI), updating the persistent policy succeeds, but for the current process the previous process-level policy remains in effect.

Set-ExecutionPolicy commendably tries to alert the user to that fact, but instead of issuing a warning, it emits a statement-terminating error.

You can tell from the wording of the message alone that this shouldn't be an error (emphasis added):

Set-ExecutionPolicy: PowerShell updated your execution policy successfully, but the setting is overridden by a policy defined at a more specific scope. Due to the override, your shell will retain its current effective execution policy of Restricted. Type "Get-ExecutionPolicy -List" to view your execution policy settings. For more information please see "Get-Help Set-ExecutionPolicy".

This behavior gets in the way of a common idiom:

# Triggers error
pwsh -ExecutionPolicy Bypass -c 'Set-ExecutionPolicy -Scope CurrentUser RemoteSigned -Force; ...'

Steps to reproduce

On Windows:

# This is is the same as calling the CLI with -ExecutionPolicy Bypass
Set-ExecutionPolicy -Scope Process Bypass -Force

try {
  Set-ExecutionPolicy -Scope CurrentUser RemoteSigned -Force
} catch {
  "Error unexpectedly thrown: $_"
}

Expected behavior

The code should run without an error, and a warning should be shown.

Actual behavior

A statement-terminating error occurs in the 2nd Set-ExecutionPolicy call, which triggers the catch block:

Error unexpectedly thrown: PowerShell updated your execution policy successfully, but the setting is overridden by a policy defined at a more specific scope.  Due to the override, your shell will retain its current effective execution policy of AllSigned. Type "Get-ExecutionPolicy -List" to view your execution policy settings. For more information please see "Get-Help Set-ExecutionPolicy".

Environment data

PowerShell Core 7.0.0
@mklement0 mklement0 added the Issue-Question ideally support can be provided via other mechanisms, but sometimes folks do open an issue to get a label Mar 5, 2020
@mklement0 mklement0 changed the title Set-ExecutionPolicy reports statement-terminating instead of warning Set-ExecutionPolicy reports statement-terminating error instead of warning Mar 5, 2020
@iSazonov iSazonov added the WG-Security security related areas such as JEA label Mar 5, 2020
@iSazonov
Copy link
Collaborator

iSazonov commented Mar 5, 2020

/cc @TravisEz13 @PaulHigin Please make a conclusion.

@PaulHigin
Copy link
Contributor

Well, it is pretty clear from the code that whoever implemented this felt this should be a terminating error. My guess is that this was done to support scripts where changing the execution policy setting is critical for the script to succeed. So even if the 'policy' is successfully changed at a specific scope, but the actual state doesn't change because some other scope overrides it, then it is an error.

It seems like it would have been better to have separate cmdlets that had clear responsibility for affecting policy and for affecting current state. But changing this now would be a breaking change (unless we propose new cmdlets and deprecate the old).

Feel free to add the 'Review-Committee' tag if you feel this should be considered.

@vexx32
Copy link
Collaborator

vexx32 commented Mar 5, 2020

@PaulHigin sure, but there's a reason -WarningAction exists.

I dont think a situation where "maybe it could be critical" calls for a terminating error all the time. 🤷‍♂

@PaulHigin
Copy link
Contributor

If a cmdlet's primary action fails then that should be a terminating error. The problem is that this cmdlet does two primary things.

@vexx32
Copy link
Collaborator

vexx32 commented Mar 5, 2020

I don't think that's really the case. It does one thing, at multiple possible scopes. If the action taken on the scope targeted succeeds... then it's done its job. The fact that there may be an overriding scope that nullifies the result of that action is, in essence, metadata. A warning sounds the best choice in my opinion.

@iSazonov iSazonov added the Breaking-Change breaking change that may affect users label Mar 5, 2020
@mklement0
Copy link
Contributor Author

mklement0 commented Mar 5, 2020

Well put, @vexx32.

I think that without backward-compatibility considerations, that is the way to go.
To also, implicitly update the Process scope is definitely not the primary purpose when you target -Scope CurrentUser or -Scope LocalMachine.
(This secondary action generally makes sense, but isn't even documented as such.)

As for it being a breaking change:

Hypothetically, someone who relied on a try / catch with a throw in the catch block would be affected, as that is the only way to abort the script - given that statement-terminating errors do not abort the script as a whole.

Much more likely, try / catch has been used without throw, simply to silence the unexpected error, because the typical use case I've seen is the following:

pwsh -ExecutionPolicy Bypass -c 'Set-ExecutionPolicy -Scope CurrentUser RemoteSigned -Force; ...'

This is a perfectly reasonable command, yet it triggers the statement-terminating error and requires a try / catch simply to silence it.
(Note that, unlike what I've originally said in the OP (since fixed), the error occurs irrespective of whether the persistent policy being set is more or less restrictive than the effective process-level one.)

In short: I would consider this a Bucket 3: Unlikely Grey Area change.

@mklement0

This comment has been minimized.

@TravisEz13 TravisEz13 added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Mar 5, 2020
@TravisEz13
Copy link
Member

Tagging for committee review for @vexx32 and @mklement0 's reasoning to be reviewed

@joeyaiello
Copy link
Contributor

@PowerShell/powershell-committee discussed this and we we're worried about potential breaking changes in an area that's so widely depended upon by the entire ecosystem. It's likely that this try/catch has been deployed very widely, and even if the fix to work around that break is simple, it could be in unmaintained areas of automation.

The issue that's really being discussed is whether or not folks depend on (and/or expect) the behavior of Set-ExecutionPolicy to set both the scope specified and the process scope. @BrucePay and @JamesWTruher both raised the fact that the cmdlets were designed and communicated like this early on, as @PaulHigin intuited from the source code.

We do agree that the documentation and/or error message could be improved to be clear about the intended behavior of the cmdlet and why the error is being thrown (specifically that the Process level execution policy could not be changed).

We discussed and ultimately rejected the idea of a Switch parameter (something like a -PolicyOnly) as it would simply add complexity to the cmdlet with no real benefit (this is why we have GP).

We also raised that it would be useful to have some cmdlets that help set configuration values in the powershell.config.json so that you could easily do something like Set-PowerShellConfigurationValue -ExecutionPolicy Bypass and have it only be reflected in the config.

@vexx32
Copy link
Collaborator

vexx32 commented Mar 11, 2020

@PowerShell/powershell-committee discussed this and we we're worried about potential breaking changes in an area that's so widely depended upon by the entire ecosystem. It's likely that this try/catch has been deployed very widely, and even if the fix to work around that break is simple, it could be in unmaintained areas of automation.

🤔 in most cases where automation is concerned, this cmdlet is not often used from what I've seen. Most tools that are running powershell use the -ExecutionPolicy Bypass mode on the executable itself.

Even if we assume that a decent portion of folks are running the cmdlet itself where the issue occurs, for the proposed change to break something, the environment itself would have to change. (Unless something is relying on not being able to change the execution policy, which... seems like a contrived scenario even at the most optimistic assessment I can muster.) That being the case would be a problem regardless of whether or not this change is taken, so the only time this impacts an automation scenario is it starting to fail... which would happen anyway, only more severely and slightly more obviously without the proposed change.

And that, inherently, means the code is being maintained, or at least the server / other system it's on is being updated, and something is bound to break or require a bit of fixing from time to time regardless.

I'm really not seeing any particular value in leaving things the way they are here, and nor do I see a lot of potential value in further complicating the executionpolicy settings. 🤷‍♂

@SteveL-MSFT
Copy link
Member

@PowerShell/powershell-committee continued discussing this and agree that we don't believe the proposed value justifies the breaking change

@SteveL-MSFT SteveL-MSFT added Committee-Reviewed PS-Committee has reviewed this and made a decision Resolution-Answered The question is answered. and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Mar 18, 2020
@iSazonov iSazonov added Resolution-By Design The reported behavior is by design. Resolution-Answered The question is answered. and removed Resolution-Answered The question is answered. labels Mar 19, 2020
@SeeminglyScience
Copy link
Collaborator

🤔 in most cases where automation is concerned, this cmdlet is not often used from what I've seen. Most tools that are running powershell use the -ExecutionPolicy Bypass mode on the executable itself.

You'd be surprised unfortunately. I've seen scenarios where the entirety of a product's generated code starts with Set-ExecutionPolicy in a try/catch with a different code path for failures. Maybe because -ExecutionPolicy Bypass is a no-op (or at least silently(?) fails) when MachinePolicy or UserPolicy is defined.

@ghost
Copy link

ghost commented Mar 24, 2020

This issue has been marked as answered and has not had any activity for 1 day. It has been closed for housekeeping purposes.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking-Change breaking change that may affect users Committee-Reviewed PS-Committee has reviewed this and made a decision Issue-Question ideally support can be provided via other mechanisms, but sometimes folks do open an issue to get a Resolution-Answered The question is answered. Resolution-By Design The reported behavior is by design. WG-Security security related areas such as JEA
Projects
None yet
Development

No branches or pull requests

8 participants