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

Add spec for proxy support #4152

Merged
merged 8 commits into from
Mar 15, 2024
Merged

Add spec for proxy support #4152

merged 8 commits into from
Mar 15, 2024

Conversation

florelis
Copy link
Member

@florelis florelis commented Feb 7, 2024

Adding spec for #190

Microsoft Reviewers: Open in CodeFlow

@florelis florelis requested a review from a team as a code owner February 7, 2024 22:34

This comment has been minimized.

New Group Policy will also be added for IT admins to control the use of proxies.
The policies will be similar to those we already have for sources, so that a specific proxy can be required or only a predefined set of proxies can be allowed.

Proxies will not be used for the configuration features for now.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think if we do the proxy configuration at wininet, DO, restclient level, then winget configuration should already be covered. Is there something I missed?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not familiar with the configuration code so I may be wrong, but the way I understand it is that that part of the code is mostly independent of the "package manager" side of things and some of it is written in .net. That's why I didn't include it in this.

I also don't know what network connections are done on that side. The only one that comes to mind is downloading the PS modules for each resource. If that's the only one, I don't see much case on going through the work of making it available on that side

Copy link
Member

Choose a reason for hiding this comment

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

If PowerShell allows control over a proxy, then we could tunnel settings along to it. Currently, the proxy would only affect the ability to retrieve a configuration document from the internet.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm fine with extending the use of proxies to configuration, but it doesn't make much sense to me if it will only affect retrieving the configuration file.

The Install-Module cmdlet has a -Proxy argument but it "ignores this parameter since it's not supported by Install-PSResource," so it doesn't really help.

Apparently we could also set the WebRequest.DefaultWebProxy property to set it for the whole PSSession, but we'd need to try it to be sure it works for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

btw, I'm fine with leaving configuration as future. I was just asking a naive question and I did not mean to add scope on this work.

doc/specs/#190 - Proxy Support.md Outdated Show resolved Hide resolved

This comment has been minimized.

yao-msft
yao-msft previously approved these changes Feb 9, 2024
doc/specs/#190 - Proxy Support.md Show resolved Hide resolved
doc/specs/#190 - Proxy Support.md Show resolved Hide resolved
doc/specs/#190 - Proxy Support.md Outdated Show resolved Hide resolved
doc/specs/#190 - Proxy Support.md Outdated Show resolved Hide resolved
Things we may want to consider in the future:
* Extend support for proxies to the Configuration feature
* Add proxy support to the COM API
* Add support for proxies that require authentication
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe there is an option to use DefaultNetworkCredentials when creating a proxy connection. Will this / should this be used in the initial implementation to enable domain-joined accounts to use their domain proxy? Just thinking that it may reduce the need to fully support authentication while providing a solution that addresses many enterprise security items


Proxies will not be used for the configuration features for now.

If a proxy is configured, it will be used for accessing sources and downloading installers.
Copy link
Member Author

Choose a reason for hiding this comment

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

I started implementing the feature and just now realized that this is not possible in all cases the way we currently do things. I was assuming it would be basically what @eternalphane did in #1776, i.e., adding the proxy argument in the call to InternetOpen(), but we have several other ways of fetching remote content and not all of them expose functionality to specify proxy.

  • Delivery Optimization cannot use a custom proxy, only the system-wide one.
  • wininet can use a custom proxy (that's InternetOpen()), but we basically only use it to download installers.
  • The MSIX deployment APIs take a URI and have no option to configure a custom proxy. (I was assuming they took a stream and I could pass the proxied stream, but was wrong..)
  • cpprestsdk can take a custom proxy to use
  • Windows.Web.Http.HttpClient that we use for getting headers (for source update), downloading source MSIX packages and some other things, only allows us to use the system-wide proxy, not to set a custom one.

So, we can implement the feature for REST API requests and for downloading non-msix installers with wininet.
We definitely cannot implement the feature for using DO, or for MSIX installers (unless we download them first, but then we lose streaming installs).
And for the things that use HttpClient, we can't unless we decide to re-implement it using wininet or something else.

Would we be comfortable with saying "proxy is only used for non-MSIX installers (where it also forces the use of wininet over DO) and REST sources"? Or how else could we move the feature forward?

Copy link
Contributor

Choose a reason for hiding this comment

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

@florelis these are fantastic call-outs.

I think we should proceed with an experimental feature for proxy with the parts we can implement, and clearly state and link to the remaining details in Issues here (even if they are just to track work external to WinGet so the community can see as they are resolved).

Those other issues should be referenced in "Future Considerations" with links to the issues and we can continue to make progress over time. I have a fairly strong belief that anything we can implement will add value, and it will just continue getting better as we are able to track and close any remaining gaps.

Copy link
Member

Choose a reason for hiding this comment

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

  • wininet can use a custom proxy (that's InternetOpen()), but we basically only use it to download installers.

It is used to download things other than installers, like manifests and the index. Installers should be using DO.

Comment on lines 47 to 53
To configure the default proxy, a new `proxy` subcommand will be added to the `settings` command, with options to `set` and `reset` the default.
This will require admin privileges and does not require `ProxyCommandLineArgument` to be enabled.

```
> winget settings proxy set https://127.0.0.1:2345
> winget settings proxy reset
```
Copy link
Member Author

Choose a reason for hiding this comment

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

The way I actually implemented this in #4203 was having set and reset subcommands for settings so that we can do
winget settings set [--setting] <SomeAdminSetting> [--value] <SomeValue> or winget settings set [--setting] <SomeAdminSetting>. Currently the only admin setting that can be used there would be DefaultProxy, but I think it could be extensible.

florelis added a commit that referenced this pull request Mar 14, 2024
For #190
See spec on #4152 
See #1776 for a related PR for the feature with the core implementation
for proxies in wininet

This PR adds basic support for using proxies. Most of the changes are
for enabling the configuration and blocking of the feature. This feature
will be gated behind an experimental feature setting

* Added Group Policy and Admin settings for enabling/disabling the use
of proxy CLI arguments and for setting a default proxy.
  + Pending: Internal review for new Group Policy
+ Extended `AdminSettings` to support settings with string values,
instead of only bool flags. The implementation is mostly a copy of the
bool case. In the future we should look back at it to reduce duplication
of code.
+ Added a `set` subcommand to `settings` that can set the admin settings
* Added CLI arguments to select a proxy on each different invocation of
winget, or to disable the use of a default one.
* Updated calls to wininet and cpprestsdk to use the provided proxy, and
added plumbing to get the arguments from the command line to the point
of use.
* Changed the flow around downloads to force winget to use proxies if
available.

Manually tested on a VM using mitmproxy
Pending: Adding automated tests tests.

Co-authored-by: yao-msft <50888816+yao-msft@users.noreply.github.com>
yao-msft
yao-msft previously approved these changes Mar 15, 2024
@florelis florelis merged commit 2fece93 into microsoft:master Mar 15, 2024
5 checks passed
@florelis florelis deleted the proxy-spec branch March 15, 2024 18:25
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.

None yet

5 participants