Skip to content

Commit

Permalink
Merge pull request from GHSA-jq3w-9mgf-43m4
Browse files Browse the repository at this point in the history
* init

* feat: add non-dev-mode verification for IP addresses

* fix: static_checks

* feat: add tests

* cleanup

* feat: add better error messaging if the hostname can't resolve

* fix: docstring

* fix: handle special cases for templated values

* feat: extract host checking and apply it to the authenticated client class

* Fixing the values being passed into deny_unsafe_hosts and moving the check to before the send is executed

* Removing unused import

* Fixing typo and updating change log

---------

Co-authored-by: Adrian Galvan <[email protected]>
  • Loading branch information
2 people authored and Kelsey-Ethyca committed Oct 22, 2023
1 parent 3231d19 commit cd344d0
Show file tree
Hide file tree
Showing 5 changed files with 76 additions and 5 deletions.
21 changes: 21 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,27 @@ The types of changes are:

## [Unreleased](https://github.com/ethyca/fides/compare/2.22.0...main)

### Added
- Added support for 3 additional config variables in Fides.js: fidesEmbed, fidesDisableSaveApi, and fidesTcString [#4262](https://github.com/ethyca/fides/pull/4262)
- Added support for fidesEmbed, fidesDisableSaveApi, and fidesTcString to be passed into Fides.js via query param, cookie, or window object [#4297](https://github.com/ethyca/fides/pull/4297)

### Fixed
- Cleans up CSS for fidesEmbed mode [#4306](https://github.com/ethyca/fides/pull/4306)

### Added
- Added a `FidesPreferenceToggled` event to Fides.js to track when user preferences change without being saved [#4253](https://github.com/ethyca/fides/pull/4253)
- Add AC Systems to the TCF Overlay under Vendor Consents section [#4266](https://github.com/ethyca/fides/pull/4266/)

### Changed
- Derive cookie storage info, privacy policy and legitimate interest disclosure URLs, and data retention data from the data map instead of directly from gvl.json [#4286](https://github.com/ethyca/fides/pull/4286)

### Fixed
- Stacks that do not have any purposes will no longer render an empty purpose block [#4278](https://github.com/ethyca/fides/pull/4278)
- Forcing hidden sections to use display none [#4299](https://github.com/ethyca/fides/pull/4299)

### Security
- Added hostname checks for external SaaS connector URLs [CVE](https://github.com/ethyca/fides/security/advisories/GHSA-jq3w-9mgf-43m4)

## [2.22.0](https://github.com/ethyca/fides/compare/2.21.0...2.22.0)

### Added
Expand Down
10 changes: 9 additions & 1 deletion src/fides/api/service/connectors/saas/authenticated_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from functools import wraps
from time import sleep
from typing import TYPE_CHECKING, Any, Callable, List, Optional, Union
from urllib.parse import urlparse

from loguru import logger
from requests import PreparedRequest, Request, Response, Session
Expand All @@ -20,6 +21,7 @@
RateLimiterPeriod,
RateLimiterRequest,
)
from fides.api.util.saas_util import deny_unsafe_hosts
from fides.config import CONFIG

if TYPE_CHECKING:
Expand Down Expand Up @@ -195,7 +197,7 @@ def send(
Builds and executes an authenticated request.
Optionally ignores:
- all non-2xx/3xx responses if ignore_errors is set to True
- no non-2xx/3xx repsones if ignore_errors is set to False
- no non-2xx/3xx responses if ignore_errors is set to False
- specific non-2xx/3xx responses if ignore_errors is set to a list of status codes
"""
rate_limit_requests = self.build_rate_limit_requests()
Expand All @@ -204,6 +206,12 @@ def send(
prepared_request: PreparedRequest = self.get_authenticated_request(
request_params
)
if not prepared_request.url:
raise ValueError("The URL for the prepared request is missing.")

# extract the hostname from the complete URL and verify its safety
deny_unsafe_hosts(urlparse(prepared_request.url).netloc)

response = self.session.send(prepared_request)

log_request_and_response_for_debugging(
Expand Down
26 changes: 25 additions & 1 deletion src/fides/api/util/saas_util.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@

import json
import re
import socket
from collections import defaultdict, deque
from typing import Any, Dict, List, Optional, Set, Tuple
from ipaddress import IPv4Address, IPv6Address, ip_address
from typing import Any, Dict, List, Optional, Set, Tuple, Union

import pydash
import yaml
Expand All @@ -15,6 +17,7 @@
from fides.api.models.privacy_request import PrivacyRequest
from fides.api.schemas.saas.saas_config import SaaSRequest
from fides.api.schemas.saas.shared_schemas import SaaSRequestParams
from fides.config import CONFIG
from fides.config.helpers import load_file

FIDESOPS_GROUPED_INPUTS = "fidesops_grouped_inputs"
Expand All @@ -24,6 +27,27 @@
CUSTOM_PRIVACY_REQUEST_FIELDS = "custom_privacy_request_fields"


def deny_unsafe_hosts(host: str) -> str:
"""
Verify that the provided host isn't a potentially unsafe one.
WARNING: IPv6 is _not_ supported and will throw an exception!
"""
if CONFIG.dev_mode:
return host

try:
host_ip: Union[IPv4Address, IPv6Address] = ip_address(
socket.gethostbyname(host)
)
except socket.gaierror:
raise ValueError(f"Failed to resolve hostname: {host}")

if host_ip.is_link_local or host_ip.is_loopback:
raise ValueError(f"Host '{host}' with IP Address '{host_ip}' is not safe!")
return host


def load_yaml_as_string(filename: str) -> str:
yaml_file = load_file([filename])
with open(yaml_file, "r", encoding="utf-8") as file:
Expand Down
File renamed without changes.
24 changes: 21 additions & 3 deletions tests/ops/service/connectors/saas/test_authenticated_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ def test_connection_config(test_saas_config) -> ConnectionConfig:
def test_saas_request() -> SaaSRequestParams:
return SaaSRequestParams(
method=HTTPMethod.GET,
path="test_path",
path="/test_path",
query_params={},
)

Expand All @@ -55,22 +55,40 @@ def test_authenticated_client(
test_connection_config, test_client_config
) -> AuthenticatedClient:
return AuthenticatedClient(
"https://test_uri", test_connection_config, test_client_config
"https://ethyca.com", test_connection_config, test_client_config
)


@pytest.mark.unit_saas
class TestAuthenticatedClient:
@mock.patch.object(Session, "send")
def test_client_returns_ok_response(
self, send, test_authenticated_client, test_saas_request
self,
send,
test_authenticated_client,
test_saas_request,
test_config_dev_mode_disabled,
):
test_response = Response()
test_response.status_code = 200
send.return_value = test_response
returned_response = test_authenticated_client.send(test_saas_request)
assert returned_response == test_response

@pytest.mark.parametrize(
"ip_address", ["localhost", "127.0.0.1", "169.254.0.1", "169.254.169.254"]
)
def test_client_denied_url(
self,
test_authenticated_client: AuthenticatedClient,
test_saas_request,
test_config_dev_mode_disabled,
ip_address,
):
test_authenticated_client.uri = f"https://{ip_address}"
with pytest.raises(ConnectionException):
test_authenticated_client.send(test_saas_request)

@mock.patch.object(Session, "send")
def test_client_retries_429_and_throws(
self, send, test_authenticated_client, test_saas_request
Expand Down

0 comments on commit cd344d0

Please sign in to comment.