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

Group Policy update with granular settings to selectively allow/disallow entry points WinGet CLI, WinGet InProc COM, WinGet Out-Of-Proc #3491

Conversation

Madhusudhan-MSFT
Copy link
Contributor

@Madhusudhan-MSFT Madhusudhan-MSFT commented Aug 1, 2023

Group Policy update with granular settings to selectively allow/disallow entry points WinGet CLI, WinGet InProc COM, WinGet Out-Of-Proc

Changes included :

  • New Group Policy called "Enable Windows Package Manager" exposes sub settings
    • WinGet CLI
    • WinGet InProcess COM
    • WinGet OutOfProcess COM to control different WinGet endpoints specified which works in combination with V1 "EnableAppInstaller" Policy. i.e when "Enable Windows Package Manager" Policy is set it has precedence over "EnableAppInstaller" Policy & when NotConfigured the default behavior for "EnableAppInstaller" Policy gets applied.
  • E2E Tests for CLI scenarios
  • E2E Tests for OutOfProc call scenario

[How Tested:]

  • Applied new group policy adml/admx template onto test machine
  • Built & Deployed WinGetDev test package executed all the applicable scenarios to be gated by the policy
  • InProc COM - Used sample C# dotnet framework project that attempts to create InProc COM object using CLSIDs with different InProc COM Policy combination and ensured they are now gated by new policy in combination with EnableAppInstaller
  • Executed Out-Of-Proc tests part of this change and ensured they all behave as expected.

Microsoft Reviewers: codeflow:open?pullrequest=#3491

…low entry proints WinGet CLI, WinGet InProc COM, WinGet Out-Of-Proc

Changes included :
- New Group Policy called "Enable Windows Package Manager" exposes sub settings
  - WinGet CLI
  - WinGet InProcess COM
  - WinGet OutOfProcess COM
  to control different WinGet endpoints specified which works in combination with V1 "EnableAppInstaller" Policy.
  i.e when  "Enable Windows Package Manager" Policy is set it has precedence over "EnableAppInstaller" Policy &
  when NotConfigured the default behavior for "EnableAppInstaller" Policy gets applied.
 - E2E Tests for CLI scenarios
 - E2E Tests for OutOfProc call scenario

 [How Tested:]
 - Applied new group policy adml/admx template onto test machine
 - Built & Deployed WinGetDev test package executed all the applicable scenarios to be gated by the policy
 - InProc COM - Used sample C# dotnet framework project that attempts to create InProc COM object using CLSIDs
   with different InProc COM Policy combination and ensured they are now gated by new policy in combination with EnableAppInstaller
 - Executed Out-Of-Proc tests part of this change and ensured they all behave as expected.
@Madhusudhan-MSFT Madhusudhan-MSFT requested a review from a team as a code owner August 1, 2023 22:41
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@Madhusudhan-MSFT Madhusudhan-MSFT changed the title Group Policy update with granular settings to selectively allow/disallow entry proints WinGet CLI, WinGet InProc COM, WinGet Out-Of-Proc Group Policy update with granular settings to selectively allow/disallow entry points WinGet CLI, WinGet InProc COM, WinGet Out-Of-Proc Aug 2, 2023
@yao-msft
Copy link
Contributor

yao-msft commented Aug 2, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Madhusudhan-MSFT
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 3491 in repo microsoft/winget-cli

@Madhusudhan-MSFT Madhusudhan-MSFT marked this pull request as draft August 2, 2023 23:00
<string id="EnableWindowsPackageManager">Enable Windows Package Manager</string>
<string id="EnableWindowsPackageManagerExplanation">This policy determines the access level of apps and services to the Windows Package Manager COM APIs and WinGet CLI, which can be used to install or configure apps on your device.

By disabling this policy, you prevent any app or service from using the Windows Package Manager COM APIs and WinGet CLI to install or configure apps on your device.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I'd try to stick to the same wording as in the other policies. "If you enable this setting" instead of "By enabling this policy"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are in discussion to revise this policy entirely. All these policy details are subject to change as part of the revision. I hope we can accommodate all these feedback part of our revision scheme.

@@ -106,6 +122,11 @@ If you disable or do not configure this setting, users will not be able to insta
<presentation id="AllowedSources">
<listBox refId="AllowedSources" required="false">Allowed Sources: </listBox>
</presentation>
<presentation id="EnableWindowsPackageManagerExplanation">
<checkBox refId="EnableWinGetCLI" defaultChecked="true">WinGet CLI</checkBox>
<checkBox refId="EnableWinGetInProcessCOM" defaultChecked="true">WinGet InProcess COM</checkBox>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is "InProcess" and "OutOfProcess" the way it is usually written? Would a translator translate it or leave it as is?


By leaving this policy as not configured, you allow any app or service to use the Windows Package Manager COM APIs to install apps on your device.

This policy will override the "Enable App Installer" policy.</string>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing this only overrides when it is set? It would be weird if it overrode a "enabled" with a "not configured"

Comment on lines +153 to +160
<boolean id="EnableWinGetCLI" valueName="EnableWinGetCLI">
<trueValue>
<decimal value="1"></decimal>
</trueValue>
<falseValue>
<decimal value="0"></decimal>
</falseValue>
</boolean>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which one is the default?

<decimal value="0"></decimal>
</falseValue>
</boolean>
</elements>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will we be able to add more elements in the future?

Comment on lines +848 to +859
PolicyState enableWinGetCLIPolicyState = Settings::GroupPolicies().GetState(Settings::TogglePolicy::Policy::WinGetCLI);

// Block any CLI execution if WinGetPackageManager - EnableWinGetCLI Policy is disabled.
if (enableWinGetCLIPolicyState == PolicyState::Disabled)
{
AICLI_LOG(CLI, Error, << "WinGet is disabled by group policy");
throw GroupPolicyException(Settings::TogglePolicy::Policy::WinGetCLI);
}

// Block any execution if winget (EnableAppInstaller) is disabled by policy & the new policy WinGetPackageManager isn't configured.
// Override the function to bypass this.
if (!Settings::GroupPolicies().IsEnabled(Settings::TogglePolicy::Policy::WinGet))
if (enableWinGetCLIPolicyState == PolicyState::NotConfigured
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to put this logic in the GroupPolicies struct so we don't have to deal with the override logic here?

@Madhusudhan-MSFT Madhusudhan-MSFT deleted the user/masudars/CLI_InProcCOM_OutOfProcCOM_GroupPolicy_Support branch August 4, 2023 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants