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.

Sign up for GitHub

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

Jump to bottom

SNOW-694457: Fix leaking HTTP_PROXY and HTTPS_PROXY environment variables #1398

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

SNOW-694457: Fix leaking HTTP_PROXY and HTTPS_PROXY environment variables #1398

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b9d84af
Fix leaking proxy env vars
ozturkberkay Jan 15, 2023
6b54477
linting fixes
sfc-gh-mkeller Aug 20, 2024
dadcb59
add changelog
sfc-gh-mkeller Aug 20, 2024
d71ab88
add skipolddriver marker
sfc-gh-mkeller Aug 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions src/snowflake/connector/connection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
from cryptography.hazmat.primitives import serialization
from cryptography.hazmat.primitives.asymmetric.rsa import RSAPrivateKey

from . import errors, proxy
from . import errors
from ._query_context_cache import QueryContextCache
from .auth import (
FIRST_PARTY_AUTHENTICATORS,
Expand Down Expand Up @@ -927,16 +927,16 @@ def __open_connection(self):
use_numpy=self._numpy, support_negative_year=self._support_negative_year
)

proxy.set_proxies(
self.proxy_host, self.proxy_port, self.proxy_user, self.proxy_password
)

self._rest = SnowflakeRestful(
host=self.host,
port=self.port,
protocol=self._protocol,
inject_client_pause=self._inject_client_pause,
connection=self,
proxy_host=self.proxy_host,
proxy_port=self.proxy_port,
proxy_user=self.proxy_user,
proxy_password=self.proxy_password,
)
logger.debug("REST API object was created: %s:%s", self.host, self.port)

Expand Down
7 changes: 7 additions & 0 deletions src/snowflake/connector/network.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@
ServiceUnavailableError,
TooManyRequests,
)
from .proxy import get_proxy_url
from .sqlstate import (
SQLSTATE_CONNECTION_NOT_EXISTS,
SQLSTATE_CONNECTION_REJECTED,
Expand Down Expand Up @@ -363,9 +364,14 @@ def __init__(
protocol: str = "http",
inject_client_pause: int = 0,
connection: SnowflakeConnection | None = None,
proxy_host: str | None = None,
proxy_port: str | None = None,
proxy_user: str | None = None,
proxy_password: str | None = None,
) -> None:
self._host = host
self._port = port
self._proxy = get_proxy_url(proxy_host, proxy_port, proxy_user, proxy_password)
self._protocol = protocol
self._inject_client_pause = inject_client_pause
self._connection = connection
Expand Down Expand Up @@ -1160,6 +1166,7 @@ def make_requests_session(self) -> Session:
s.mount("http://", ProxySupportAdapter(max_retries=REQUESTS_RETRY))
s.mount("https://", ProxySupportAdapter(max_retries=REQUESTS_RETRY))
s._reuse_count = itertools.count()
s.proxies = {"http": self._proxy, "https": self._proxy}
return s

@contextlib.contextmanager
Expand Down
49 changes: 16 additions & 33 deletions src/snowflake/connector/proxy.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,43 +5,26 @@

from __future__ import annotations

import os


def set_proxies(
def get_proxy_url(
proxy_host: str | None,
proxy_port: str | None,
proxy_user: str | None = None,
proxy_password: str | None = None,
) -> dict[str, str] | None:
"""Sets proxy dict for requests."""
PREFIX_HTTP = "http://"
PREFIX_HTTPS = "https://"
proxies = None
) -> str | None:
http_prefix = "http://"
https_prefix = "https://"

if proxy_host and proxy_port:
if proxy_host.startswith(PREFIX_HTTP):
proxy_host = proxy_host[len(PREFIX_HTTP) :]
elif proxy_host.startswith(PREFIX_HTTPS):
proxy_host = proxy_host[len(PREFIX_HTTPS) :]
if proxy_user or proxy_password:
proxy_auth = "{proxy_user}:{proxy_password}@".format(
proxy_user=proxy_user if proxy_user is not None else "",
proxy_password=proxy_password if proxy_password is not None else "",
)
if proxy_host.startswith(http_prefix):
host = proxy_host[len(http_prefix) :]
elif proxy_host.startswith(https_prefix):
host = proxy_host[len(https_prefix) :]
else:
proxy_auth = ""
proxies = {
"http": "http://{proxy_auth}{proxy_host}:{proxy_port}".format(
proxy_host=proxy_host,
proxy_port=str(proxy_port),
proxy_auth=proxy_auth,
),
"https": "http://{proxy_auth}{proxy_host}:{proxy_port}".format(
proxy_host=proxy_host,
proxy_port=str(proxy_port),
proxy_auth=proxy_auth,
),
}
os.environ["HTTP_PROXY"] = proxies["http"]
os.environ["HTTPS_PROXY"] = proxies["https"]
return proxies
host = proxy_host
auth = (
f"{proxy_user or ''}:{proxy_password or ''}@"
if proxy_user or proxy_password
else ""
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of doing proxy_user or '' and proxy_password or '' I'd personally prefer to set the default values of the function to those. That'd end up changing this code to:

        auth = (
            f"{proxy_user}:{proxy_password}@"
            if proxy_user or proxy_password
            else ""
        )

I did get caught out by all those ORs, but it doesn't actually change what the code does.
This is more of my nitpick

return f"{http_prefix}{auth}{host}:{proxy_port}"
10 changes: 6 additions & 4 deletions test/integ/test_connection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -524,6 +524,8 @@ def test_invalid_account_timeout():

@pytest.mark.timeout(15)
def test_invalid_proxy(db_parameters):
http_proxy = os.environ.get("HTTP_PROXY")
https_proxy = os.environ.get("HTTPS_PROXY")
with pytest.raises(OperationalError):
snowflake.connector.connect(
protocol="http",
Expand All @@ -532,13 +534,13 @@ def test_invalid_proxy(db_parameters):
password=db_parameters["password"],
host=db_parameters["host"],
port=db_parameters["port"],
login_timeout=5,
login_timeout=0,
proxy_host="localhost",
proxy_port="3333",
)
# NOTE environment variable is set if the proxy parameter is specified.
del os.environ["HTTP_PROXY"]
del os.environ["HTTPS_PROXY"]
# Proxy environment variables should not change
assert os.environ.get("HTTP_PROXY") == http_proxy
assert os.environ.get("HTTPS_PROXY") == https_proxy


@pytest.mark.timeout(15)
Expand Down
36 changes: 12 additions & 24 deletions test/unit/test_proxies.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,30 +14,18 @@
import snowflake.connector
from snowflake.connector.errors import OperationalError


def test_set_proxies():
from snowflake.connector.proxy import set_proxies

assert set_proxies("proxyhost", "8080") == {
"http": "http://proxyhost:8080",
"https": "http://proxyhost:8080",
}
assert set_proxies("http://proxyhost", "8080") == {
"http": "http://proxyhost:8080",
"https": "http://proxyhost:8080",
}
assert set_proxies("http://proxyhost", "8080", "testuser", "testpass") == {
"http": "http://testuser:testpass@proxyhost:8080",
"https": "http://testuser:testpass@proxyhost:8080",
}
assert set_proxies("proxyhost", "8080", "testuser", "testpass") == {
"http": "http://testuser:testpass@proxyhost:8080",
"https": "http://testuser:testpass@proxyhost:8080",
}

# NOTE environment variable is set if the proxy parameter is specified.
del os.environ["HTTP_PROXY"]
del os.environ["HTTPS_PROXY"]
def test_get_proxy_url():
from snowflake.connector.proxy import get_proxy_url

assert get_proxy_url("host", "port", "user", "password") == (
"http://user:password@host:port"
)
assert get_proxy_url("host", "port") == "http://host:port"

assert get_proxy_url("http://host", "port") == "http://host:port"
assert get_proxy_url("https://host", "port", "user", "password") == (
"http://user:password@host:port"
)


@pytest.mark.skipolddriver
Expand Down
Loading