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

SNOW-1333163: Add support for default proxy configuration #917

Open
vc-74 opened this issue Apr 17, 2024 · 13 comments
Open

SNOW-1333163: Add support for default proxy configuration #917

vc-74 opened this issue Apr 17, 2024 · 13 comments
Assignees
Labels
feature status-pr_pending_merge A PR is made and is under review status-triage_done Initial triage done, will be further handled by the driver team

Comments

@vc-74
Copy link

vc-74 commented Apr 17, 2024

What is the current behavior?

The current HttpUtil implementation allows 2 scenarios:

  • Explicitly set proxy by specifying useproxy, proxyhost... in the connection string
  • Ignore proxy parameters in which case a proxy will never be used

This does not allow the default proxy scenarios, for instance:

  • Getting proxy configuration from environment variables (HTTP_PROXY, HTTPS_PROXY, NO_PROXY...)
  • On Windows, getting proxy configuration from WinInet, which is the only scenario supporting PAC scripts AFAIK

What is the desired behavior?

Since HttpClientHandler.UseProxy defaults to true and not specifying HttpClientHandler.Proxy will use the default proxy by default:

  • If useproxy is true and proxy parameters are specified (proxyhost...) set HttpClientHandler.Proxy --> already available
  • If useproxy is false, set HttpClientHandler.UseProxy to false --> already available
  • If useproxy is true and no proxy parameters are specified (proxyhost...) leave HttpClientHandler.Proxy to its default value --> not available today

How would this improve snowflake-connector-net?

This would allow proxy settings from environment variables, PAC scripts... to be used to connect to Snowflake.

References, Other Background

@vc-74 vc-74 added the feature label Apr 17, 2024
@github-actions github-actions bot changed the title Add support for default proxy configuration SNOW-1333163: Add support for default proxy configuration Apr 17, 2024
@sfc-gh-dszmolka sfc-gh-dszmolka self-assigned this Apr 17, 2024
@sfc-gh-dszmolka
Copy link
Contributor

hi and again thank you for raising this as a separate Issue !

you're correct, currently the documented and supported way of setting proxies is configuring USEPROXY and the rest of the proxy settings like PROXYHOST and PROXYPORT (optionally, for authenticated proxies, the PROXYUSER and PROXYPASSWORD as well) in the connection settings.

we'll consider this improvement request for future plans. Of course if it's within your possibility to submit a PR too, that's greatly appreciated!

@sfc-gh-dszmolka sfc-gh-dszmolka added the status-triage_done Initial triage done, will be further handled by the driver team label Apr 17, 2024
@vc-74
Copy link
Author

vc-74 commented Apr 18, 2024

we'll consider this improvement request for future plans. Of course if it's within your possibility to submit a PR too, that's greatly appreciated!

Done

@sfc-gh-dszmolka sfc-gh-dszmolka added the status-pr_pending_merge A PR is made and is under review label Apr 18, 2024
@sfc-gh-dszmolka
Copy link
Contributor

thank you ! the team will review and advise

@pgr-rick-schulte
Copy link

Can we get this issue escalated? prior to ver. 2.0.25 Proxy/No proxy setting we're working from environment variables. Perhaps you can reenable that code? We have a number of users that have not updated to the latest because of this and now Security is concerned about vulnerabilities. Proxy Settings in the connection is not our standard or manageable practice.

@sfc-gh-dszmolka
Copy link
Contributor

I escalated this internally to the team but for future reference; for any important and production impacting issues or requests, the path for escalation is always your Snowflake account team as they have more context about your situation and also other tools at their disposal to help issues progress. Thank you for your understanding !

@vc-74
Copy link
Author

vc-74 commented Jul 22, 2024

@sfc-gh-dszmolka How long does it usually take for a PR to be reviewed?

@sfc-gh-dszmolka
Copy link
Contributor

sfc-gh-dszmolka commented Jul 22, 2024

I cannot comment on that unfortunately, but usually should not take this much time, but again, escalated it on sunday (yesterday). If you're already a Snowflake customer, I recommend to do the same , to your account team. I regret this takes so much time, not sure about the reasons - apologies !

@pgr-rick-schulte
Copy link

It's interesting that the same behavior "removal of Proxy settings locations" happened in the SF ODBC driver around the same time . I had opened an issue with the ODBC team, and they added it back in the ODBC 3.1.4 ver. for us. It was Case#00620057.
I'm guessing there is some cross coordination between you, .Net Team and the ODBC driver would this help escalate?

@sfc-gh-dszmolka
Copy link
Contributor

sfc-gh-dszmolka commented Jul 26, 2024

a quick update on this one

  • someone is actively looking into this and hope i can share some progress next week
  • identified the culprit to be a change between 2.0.25 and 2.1.0; so we can focus on that (the part where HTTPS_PROXY / HTTP_PROXY was picked up by the driver 2.0.25 and before, and is not picked up on or after 2.1.0)

@sfc-gh-dszmolka
Copy link
Contributor

also I see PR was reviewed yesterday and some comments have been left

@pgr-rick-schulte
Copy link

Thank- you for the identifying where the change occurred

@sfc-gh-dszmolka sfc-gh-dszmolka removed the status-pr_pending_merge A PR is made and is under review label Nov 26, 2024
@sfc-gh-dszmolka
Copy link
Contributor

the community PR seems to be closed by deleting the underlying dev branch

@sfc-gh-dszmolka sfc-gh-dszmolka added the status-pr_pending_merge A PR is made and is under review label Dec 10, 2024
@sfc-gh-dszmolka
Copy link
Contributor

PR with the change is here: #997 . It still needs a while, due to underlying security validation procedure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature status-pr_pending_merge A PR is made and is under review status-triage_done Initial triage done, will be further handled by the driver team
Projects
None yet
Development

No branches or pull requests

4 participants