From 7f7c730cb415f6b1b22abe5788a10e02c50778ad Mon Sep 17 00:00:00 2001 From: Yuriy Andamasov Date: Sun, 4 Oct 2026 12:29:31 +0300 Subject: http-api-client: T8955: harden verify_tls handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address the adversarial-review findings on the TLS verification change: - Finding 1: validate ApiClientConfig.verify_tls in __post_init__. Only a bool or a non-empty CA bundle path is accepted; anything else (including "", None and ints) raises ValueError. requests treats "" as falsy and would silently disable verification. Strings are never coerced: "false" is a path, so callers must pass a real bool. - Finding 2: stop calling urllib3.disable_warnings() process-wide from ApiClient.__init__. InsecureRequestWarning is now ignored only around the request itself, via warnings.catch_warnings(), and only when verify_tls is False. post() is the only request path in the client. - Finding 3: wrap OSError (raised by requests' cert_verify for a missing CA bundle path) in ApiTransportError. The handler sits after the RequestException handlers, since those subclass OSError. - Finding 4: config-sync hard-codes verify_tls = False instead of reading an unmapped 'verify_tls' key from the runtime config, whose leaf values are strings. Mapping a CLI option to a bool or CA path is T9396. 🤖 Generated by [robots](https://vyos.io) --- python/vyos/http_api_client.py | 58 +++++++++++++++++++++++++++++++++--------- 1 file changed, 46 insertions(+), 12 deletions(-) (limited to 'python') diff --git a/python/vyos/http_api_client.py b/python/vyos/http_api_client.py index f0783e3fc..88b09d740 100644 --- a/python/vyos/http_api_client.py +++ b/python/vyos/http_api_client.py @@ -16,8 +16,10 @@ # along with this library. If not, see . import json +import warnings import urllib3 import requests +from contextlib import contextmanager from typing import Optional from dataclasses import dataclass @@ -25,6 +27,21 @@ from vyos.version import get_version from vyos.template import bracketize_ipv6 +@contextmanager +def _suppress_insecure_warning(enabled: bool): + """Locally ignore urllib3's InsecureRequestWarning when enabled. + + Uses warnings.catch_warnings() so the process-wide filters are restored + on exit instead of being changed globally. + """ + if not enabled: + yield + return + with warnings.catch_warnings(): + warnings.simplefilter('ignore', urllib3.exceptions.InsecureRequestWarning) + yield + + class ApiError(Exception): """Generic VyOS HTTP API client error""" @@ -55,8 +72,21 @@ class ApiClientConfig: # True - verify against the system CA store (secure default) # False - disable verification (insecure; opt-in only) # "" - verify against a custom CA bundle file/dir + # Strings are always treated as paths: pass a real bool, not "false". verify_tls: bool | str = True + def __post_init__(self): + # requests treats an empty string as falsy and silently disables + # verification, so only accept a bool or a non-empty path string. + value = self.verify_tls + if isinstance(value, bool): + return + if isinstance(value, str) and value: + return + raise ValueError( + f'verify_tls must be a bool or a non-empty CA bundle path, got {value!r}' + ) + class ApiClient: """Small helper for talking to VyOS HTTP API using requests. @@ -82,11 +112,6 @@ class ApiClient: self._session = requests.Session() self._session.headers.update(self._DEFAULT_HEADERS) - # Only silence the insecure-request warning when verification has been - # explicitly disabled. A CA bundle path or True must keep warnings on. - if config.verify_tls is False: - urllib3.disable_warnings(urllib3.exceptions.InsecureRequestWarning) - @property def base_url(self) -> str: return f'https://{self._host}:{self._cfg.port}' @@ -121,18 +146,27 @@ class ApiClient: url = f'{self.base_url}{endpoint}' + # Only silence the insecure-request warning for this call, and only + # when verification has been explicitly disabled. + insecure = self._cfg.verify_tls is False + try: - resp = self._session.post( - url, - data=json.dumps(body), - params=params, - timeout=self._cfg.timeout, - verify=self._cfg.verify_tls, - ) + with _suppress_insecure_warning(insecure): + resp = self._session.post( + url, + data=json.dumps(body), + params=params, + timeout=self._cfg.timeout, + verify=self._cfg.verify_tls, + ) except requests.exceptions.Timeout as e: raise ApiTransportError(f'Request timed out: {e}') from e except requests.exceptions.RequestException as e: raise ApiTransportError(f'Request failed: {e}') from e + except OSError as e: + # e.g. requests' cert_verify on a missing CA bundle path. Must stay + # after the RequestException handlers: those subclass OSError. + raise ApiTransportError(f'Request failed: {e}') from e if not resp.ok: text = resp.text.strip() -- cgit v1.2.3