diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 15d2f4d..28a4bfe 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -9,17 +9,25 @@ on: branches: - main +# Default to read-only; the lint job below grants itself the write scopes +# lint-action actually needs (auto-fix commits + check-run annotations). +permissions: + contents: read + jobs: lint: name: Run black linter runs-on: ubuntu-latest + permissions: + contents: write # auto_fix: true pushes formatting commits back to the branch + checks: write # lint-action publishes results as a check run steps: - name: Check out Git repository uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5 - name: Set up Python uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5 - name: Install Python dependencies - run: pip install black + run: pip install black==26.5.1 # match the pin in requirements-dev.txt - name: Run black uses: wearerequired/lint-action@548d8a7c4b04d3553d32ed5b6e91eb171e10e7bb # v2 with: diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 365f75e..bad9d9b 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -4,9 +4,17 @@ on: tags: - 'v*' +permissions: + contents: read + jobs: deploy: runs-on: ubuntu-latest + permissions: + contents: read + # Required for PyPI Trusted Publishing (OIDC) below; no PYPI_API_TOKEN + # secret is used or needed once a trusted publisher is configured. + id-token: write steps: - uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5 @@ -18,14 +26,18 @@ jobs: - name: Install dependencies run: | - python -m pip install --upgrade pip - python -m pip install flit + python -m pip install --upgrade "pip>=26.2" # CVE-2026-8643, CVE-2026-6357, CVE-2026-13346, CVE-2026-3219 + python -m pip install flit==3.12.0 # match flit_core pin in pyproject.toml - name: Build package run: flit build - name: Publish package - uses: pypa/gh-action-pypi-publish@release/v1 - with: - user: __token__ - password: ${{ secrets.PYPI_API_TOKEN }} + # Security review item SDK-5 (PR #98): migrated from a long-lived + # PYPI_API_TOKEN to PyPI Trusted Publishing (OIDC), and the action ref + # is now SHA-pinned (it was previously the mutable `release/v1` branch). + # REQUIRES: a trusted publisher for this repo + workflow file must be + # configured on pypi.org (project Settings -> Publishing) before this + # tag push will succeed. Coordinate with the PyPI project owner first; + # keep the PYPI_API_TOKEN repo secret until that is confirmed working. + uses: pypa/gh-action-pypi-publish@dc37677b2e1c63e2034f94d8a5b11f265b73ba33 # v1.14.2 diff --git a/.github/workflows/run_tests.yml b/.github/workflows/run_tests.yml index af891d6..8b8b614 100644 --- a/.github/workflows/run_tests.yml +++ b/.github/workflows/run_tests.yml @@ -2,6 +2,11 @@ name: Run Tests on: [pull_request] +# This workflow only checks out code and runs the test suite; it never +# writes to the repo or opens PRs/issues, so read-only is sufficient. +permissions: + contents: read + jobs: build: @@ -9,7 +14,8 @@ jobs: environment: testing strategy: matrix: - python: [3.8, 3.9, "3.10", "3.11"] + # Python 3.8/3.9 dropped: fixed requests/urllib3 pins require Python >= 3.10 (work item 741117) + python: ["3.10", "3.11", "3.12"] steps: - uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5 @@ -20,8 +26,10 @@ jobs: - name: Install Tox run: | - python -m pip install --upgrade pip - pip install tox + # Upgrading pip has to happen in the outer interpreter; a pin in a + # requirements file cannot replace the running pip. + python -m pip install --upgrade "pip>=26.2" # CVE-2026-8643, CVE-2026-6357, CVE-2026-13346, CVE-2026-3219 + python -m pip install tox - name: Run Tox # Run tox using the version of Python in `PATH` diff --git a/README.md b/README.md index a64c152..71951bc 100644 --- a/README.md +++ b/README.md @@ -31,7 +31,7 @@ There are three ways in which you can authorize the `SecretServer` and `SecretSe #### Password Authorization -If using traditional `username` and `password` authentication to log in to your Secret Server either directly or through Platform, you can pass the `PasswordGrantAuthorizer` into the `SecretServer` class at instantiation. The `PasswordGrantAuthorizer` requires a `base_url`, `username`, and `password`. It optionally takes a `token_path_uri`, but defaults to `/oauth2/token` or `/identity/api/oauth2/token/xpmplatform`, depending on whether a secret server or platform is used for authentication. +If using traditional `username` and `password` authentication to log in to your Secret Server either directly or through Platform, you can pass the `PasswordGrantAuthorizer` into the `SecretServer` class at instantiation. The `PasswordGrantAuthorizer` requires a `base_url`, `username`, and `password`. It optionally takes a `token_path_uri`, but defaults to `/oauth2/token` or `/identity/api/oauth2/token/xpmplatform`, depending on whether a secret server or platform is used for authentication. It also optionally takes a `server_type` (`"secret_server"` or `"platform"`) to skip automatic server-type detection — see [Server-Type Detection](#server-type-detection). ##### With Secret Server ```python @@ -50,7 +50,7 @@ authorizer = PasswordGrantAuthorizer("https://platform.delinea.app", os.getenv(" #### Domain Authorization -To use a domain credential, use the `DomainPasswordGrantAuthorizer`. It requires a `base_url`, `username`, `domain`, and `password`. It optionally takes a `token_path_uri`, but defaults to `/oauth2/token`. It is applicable only when authentication is done using a secret server. +To use a domain credential, use the `DomainPasswordGrantAuthorizer`. It requires a `base_url`, `username`, `domain`, and `password`. It optionally takes a `token_path_uri`, but defaults to `/oauth2/token`, and a `server_type` (see [Server-Type Detection](#server-type-detection)). It is applicable only when authentication is done using a secret server. ```python from delinea.secrets.server import DomainPasswordGrantAuthorizer @@ -60,7 +60,7 @@ authorizer = DomainPasswordGrantAuthorizer("https://hostname/SecretServer", os.g #### Access Token Authorization -If you already have an `access_token` of Secret Server or Platform user, you can pass directly via the `AccessTokenAuthorizer`. The `AccessTokenAuthorizer` requires a `access_token` and `base_url`. +If you already have an `access_token` of Secret Server or Platform user, you can pass directly via the `AccessTokenAuthorizer`. The `AccessTokenAuthorizer` requires a `access_token` and `base_url`. It optionally takes a `server_type` (see [Server-Type Detection](#server-type-detection)). ##### With Secret Server ```python @@ -77,6 +77,25 @@ from delinea.secrets.server import AccessTokenAuthorizer authorizer = AccessTokenAuthorizer("AgJ1slfZsEng9bKsssB-tic0Kh8I...", "https://platform.delinea.app") ``` +#### Server-Type Detection + +Unless given an explicit `server_type`, an authorizer detects whether the `base_url` points at a Secret Server or a Platform instance by probing its health-check endpoints (`/api/v1/healthcheck` then `/health`). `AccessTokenAuthorizer` probes when it is constructed; `PasswordGrantAuthorizer` and `DomainPasswordGrantAuthorizer` probe on their first token request, so constructing one does not validate the URL. The result is cached per `base_url` for the lifetime of the process, so the probe pair normally fires once per `base_url`. `SecretServerV0` accepts the same `server_type` keyword and passes it to the authorizer it builds. + +You can skip detection entirely by passing an explicit `server_type` of either `"secret_server"` or `"platform"`. When supplied, no health-check probe is issued. This is recommended for callers that run each lookup in a fresh, short-lived process (for example, some Ansible lookup-plugin runtimes), where a fresh process cannot benefit from the in-process cache and the repeated unauthenticated probes can be rate-limited to `403` by the Delinea Platform WAF. + +```python +from delinea.secrets.server import AccessTokenAuthorizer + +# No health-check probe is issued; the type is used directly. +authorizer = AccessTokenAuthorizer( + "AgJ1slfZsEng9bKsssB-tic0Kh8I...", + "https://platform.delinea.app", + server_type="platform", +) +``` + +An explicit `server_type` applies only to the instance that supplies it and is never written to the shared cache, so it cannot affect auto-detection for other authorizers. If a `base_url` is ever re-provisioned to a different server type while a long-lived process is running, call `Authorizer.clear_server_type_cache()` to force re-detection. + ## Secret Server Cloud The SDK API requires an `Authorizer` and either a `tenant` or a `base_url`. In the case of plaform authentication, only a `base_url` is supported. @@ -85,7 +104,7 @@ The SDK API requires an `Authorizer` and either a `tenant` or a `base_url`. In t ### Useage -Instantiate the `SecretServerCloud` class with `tenant` or `base_url`, along with an `Authorizer` (when providing `tenant`, yoou may optionally include a `tld`). To retrieve a secret, pass an integer `id` to `get_secret()` which will return the secret as a JSON encoded string. +Instantiate the `SecretServerCloud` class with `tenant` or `base_url`, along with an `Authorizer` (when providing `tenant`, yoou may optionally include a `tld`). To retrieve a secret, pass an integer `id` to `get_secret()` which will return the secret as a `dict`. ##### With Secret Server ```python @@ -139,7 +158,7 @@ from delinea.secrets.server import SecretServer secret_server = SecretServer(base_url="https://platform.delinea.app", authorizer=authorizer) ``` -Secrets can be fetched using the `get_secret` method, which takes an integer `id` of the secret and, returns a `json` object: +Secrets can be fetched using the `get_secret` method, which takes an integer `id` of the secret and returns a `dict`: ```python secret = secret_server.get_secret(os.getenv("TSS_SECRET_ID")) @@ -159,7 +178,7 @@ secret = ServerSecret(**secret_server.get_secret(os.getenv("TSS_SECRET_ID"))) username = secret.fields['username'].value ``` -It is also now possible to fetch a secret by the secrets `path` using the `get_secret_by_path` method on the `SecretServer` object. This, too, returns a `json` object. +It is also now possible to fetch a secret by the secrets `path` using the `get_secret_by_path` method on the `SecretServer` object. This, too, returns a `dict`. ```python secret = secret_server.get_secret_by_path(r"TSS_SECRET_PATH") @@ -182,13 +201,56 @@ except SecretServerError as e: > Note: The `path` must be the full folder path and name of the secret. +### File Attachments + +`get_secret()` and `get_secret_by_path()` fetch file attachments by default. +Every field with a non-zero `fileAttachmentId` gets its `itemValue` replaced +with a `FileAttachment` (importable from `delinea.secrets.server`): the file's +bytes, plus `.content`, `.text` and `.encoding`. Releases up to 2.0.1 stored +the `requests.Response` there, so every other member of it — `.status_code`, +`.json()`, `.headers`, `.ok`, `.iter_content()` — now raises `AttributeError`. +`.text` prefers a strict UTF-8 decode when the server declares Latin-1, which +`requests` reports for any `text/*` body with no charset. `.filename` and +`.encoding` carry what the server sent, or `None`. + +```python +import os +import pathlib + +secret = secret_server.get_secret(os.getenv("TSS_SECRET_ID")) +downloads = pathlib.Path("downloads") +downloads.mkdir(parents=True, exist_ok=True) + +for item in secret["items"]: + if item.get("fileAttachmentId"): + # `filename` is server data: name the file yourself rather than + # joining it into a path, and do not rely on the key being present. + target = downloads / f"{secret['id']}_{item['slug']}" + target.write_bytes(item["itemValue"].content) +``` + +Use `.content` for any attachment, and `.text` only for one you know is text. +An empty attachment is falsy, like any empty `bytes`, so test +`item.get("fileAttachmentId")` rather than the value itself. Some templates +omit that key entirely, which is why the example reads it with `.get`. + +Treat the value as read-once. Every `bytes` operation on it — slicing, +concatenation, `.strip()` — returns plain `bytes` and drops `.filename`, +`.encoding` and `.text`, and two attachments with identical contents compare +equal whatever their filenames. Copy what you need out before transforming. + +`repr()` of a `FileAttachment` reports its size, not its contents, so an +attachment cannot leak through a log line. The secret's other field values are +ordinary strings, so never log the secret itself. `json.dumps()` of a fetched +secret raises on the bytes: pass `fetch_file_attachments=False` for JSON. + ## Using Self-Signed Certificates When using a self-signed certificate for SSL, the `REQUESTS_CA_BUNDLE` environment variable should be set to the path of the certificate (in `.pem` format). This will negate the need to ignore SSL certificate verification, which makes your application vunerable. Please reference the [`requests` documentation](https://docs.python.org/3/library/ssl.html) for further details on the `REQUESTS_CA_BUNDLE` environment variable, should you require it. ## Create a Build Environment (optional) -The SDK requires [Python 3.8](https://www.python.org/downloads/) or higher. +The SDK requires [Python 3.10](https://www.python.org/downloads/) or higher. First, ensure Python is in `$PATH`, then run: @@ -201,12 +263,19 @@ cd python-tss-sdk python -m venv venv . venv/bin/activate -# Install dependencies -python -m pip install --upgrade pip -pip install -r requirements.txt +# Install dependencies (runtime + test/build tooling) +python -m pip install --upgrade "pip>=26.2" +pip install -r requirements-dev.txt +``` + +Most of the suite runs offline and needs no credentials or network access: + +```shell +pytest tests/test_security_phase1.py tests/test_security_phase2.py \ + tests/test_security_phase4.py tests/test_server_detection_cache.py ``` -Valid credentials are required to run the unit tests. The credentials should be stored in environment variables or in a `.env` file: +Valid credentials are required to run the live integration tests in `tests/test_server.py`. The credentials should be stored in environment variables or in a `.env` file: ```shell export TSS_USERNAME=myusername diff --git a/SECURITY.md b/SECURITY.md new file mode 100644 index 0000000..6faf130 --- /dev/null +++ b/SECURITY.md @@ -0,0 +1,28 @@ +# Security Policy + +## Supported Versions + +Security fixes are released against the latest published version of `python-tss-sdk` on PyPI. We do not backport fixes to older minor/major versions; please upgrade to the latest release to receive security patches. + +## Reporting a Vulnerability + +If you believe you have found a security vulnerability in this SDK, please report it responsibly through Delinea's coordinated disclosure program rather than opening a public GitHub issue: + +- **Trust Portal (preferred):** +- **Email:** + +Please include: + +- A description of the vulnerability and its potential impact. +- Steps to reproduce, including a minimal code sample against this SDK if applicable. +- The SDK version (`delinea.__version__`) and Python version in use. + +Do not include real credentials, tokens, or secret values from a live Secret Server/Platform tenant in a report. + +## What to Expect + +Delinea's security team acknowledges and triages reports submitted through the channels above; response times and disclosure timelines are governed by the program terms published at . Please do not disclose a suspected vulnerability publicly until it has been addressed. + +## Scope + +This policy covers the SDK code in this repository (`delinea/secrets/server.py` and related packaging). Vulnerabilities in Secret Server, Delinea Platform, or other Delinea products should be reported through the same channels above, which will route them to the appropriate team. diff --git a/delinea/__init__.py b/delinea/__init__.py index e05db34..d4142e8 100644 --- a/delinea/__init__.py +++ b/delinea/__init__.py @@ -1,3 +1,6 @@ """The Delinea Secret Server Python SDK""" -__version__ = "2.0.1" +# 3.0.0, not 2.0.2: this line is the published version (flit reads it), and +# the branch carries three breaking changes -- the attachment ``itemValue`` +# type, requires-python >= 3.10, and the requests floor. See work item 741117. +__version__ = "3.0.0" diff --git a/delinea/secrets/server.py b/delinea/secrets/server.py index 0f26d4a..d345c17 100644 --- a/delinea/secrets/server.py +++ b/delinea/secrets/server.py @@ -14,14 +14,311 @@ secret = ServerSecret(**secret_server.get_secret(123)) """ +import codecs +import copy import json +import logging +import math import re +import sys +import warnings from abc import ABC, abstractmethod +from collections import OrderedDict +from collections.abc import Mapping from dataclasses import dataclass -from datetime import datetime, timedelta +from datetime import datetime, timedelta, timezone +from threading import Event, Lock +from urllib.parse import parse_qsl, urlsplit import requests +logger = logging.getLogger(__name__) + +# Applied to every HTTP call the SDK makes; ``requests`` has no default +# timeout, so an omitted value would let a stalled connection hang forever. +DEFAULT_REQUEST_TIMEOUT = 60 + +# Cap on how much of a server response body is echoed into an exception +# message, so a malformed/oversized response cannot flood logs and so +# exception text stays clearly distinguishable from a full response body. +_BODY_EXCERPT_LIMIT = 200 +_TRUNCATION_MARKER = "...[truncated]" + +# Cap on the server-supplied attachment filename echoed into a repr. Shorter +# than a body excerpt: it identifies the file in a log line, nothing more. +_FILENAME_EXCERPT_LIMIT = 60 + +# How long a caller waits on another thread's in-flight detection before +# probing itself. ``requests``' timeout is per socket operation, so a live +# leader may spend connect plus read on each of two probes: four, plus slack. +_DETECTION_WAIT_TIMEOUT = 4 * DEFAULT_REQUEST_TIMEOUT + 5 + +# Lifetime assumed for an access grant with no ``expires_in``. RFC 6749 makes +# the field RECOMMENDED, so both products send it and this covers only a +# non-conforming proxy; one hour is the conventional OAuth2 default. +_DEFAULT_GRANT_LIFETIME_SECONDS = 3600 + +# Ceiling on a grant lifetime. Beyond roughly this, ``now + timedelta`` +# overflows ``datetime`` and every later call would raise OverflowError. +_MAX_GRANT_LIFETIME_SECONDS = 10 * 365 * 24 * 3600 + + +def _with_query_flag(params, key, value): + """``params`` plus ``key=value``, in any form ``requests`` accepts. + + The flag is sent once and wins; a non-mapping form stays a list of pairs, + so repeated keys the caller relies on are not collapsed. + """ + if params is None or isinstance(params, Mapping): + return {**(params or {}), key: value} + if isinstance(params, bytes): + params = params.decode("utf-8", errors="replace") + if isinstance(params, str): + pairs = parse_qsl(params, keep_blank_values=True) + else: + pairs = list(params) + return [(k, v) for k, v in pairs if k != key] + [(key, value)] + + +def _join_url(base, path): + """Join ``base`` and ``path`` with exactly one slash between them. + + The one place that knows how a base URL and a path segment combine, so the + token endpoint, API root, vault call and probes cannot drift apart. + """ + return f"{base.rstrip('/')}/{path.strip('/')}" + + +def _caller_stacklevel(): + """Return the ``stacklevel`` of the first frame outside this module. + + Each wrapper adds a frame, so a constant aims the warning inside this file. + TODO(python>=3.12): ``warnings.warn(skip_file_prefixes=...)`` replaces this. + """ + level = 1 + try: + frame = sys._getframe(1) # the caller of this helper + except ValueError: # pragma: no cover - no caller frame + return 2 + while frame is not None and frame.f_globals.get("__name__") == __name__: + frame = frame.f_back + level += 1 + return level + + +def _warn_if_insecure(base_url): + """Warn when ``base_url`` does not use ``https``. + + Credentials and bearer tokens travel in plaintext otherwise; the warning is + attributed to the caller. TODO(v4.0): reject non-https, with an opt-out. + """ + try: + scheme = urlsplit(base_url).scheme + except ValueError as exc: # unclosed IPv6 bracket, NFKC-changing netloc + raise ValueError(f"base_url {base_url!r} is not a valid URL: {exc}") from exc + if scheme.lower() != "https": + warnings.warn( + f"base_url {base_url!r} does not use https; credentials and " + "bearer tokens will be sent unencrypted.", + UserWarning, + stacklevel=_caller_stacklevel(), + ) + + +def _safe_body_excerpt(text, limit=_BODY_EXCERPT_LIMIT): + """Return a length-capped excerpt of a response body for use in error + messages, marked when truncated so it's clearly not the full body.""" + if text is None: + return "" + text = str(text) + if len(text) <= limit: + return text + return text[:limit] + _TRUNCATION_MARKER + + +def _safe_body_excerpt_bytes(content, limit=_BODY_EXCERPT_LIMIT, encoding=None): + """Return a length-capped excerpt of a raw, undecoded response body. + + Slices ``4 * (limit + 1)`` bytes first and marks any body that was cut. A + declared Latin-1 yields to valid UTF-8; an unusable codec falls back to it. + """ + if not content: + return "" + if isinstance(content, str): + return _safe_body_excerpt(content, limit) + head = content[: 4 * (limit + 1)] + truncated = len(head) < len(content) + codec = encoding if isinstance(encoding, str) and encoding else "utf-8" + try: + canonical = codecs.lookup(codec).name + except (LookupError, ValueError): + # ``ValueError``: a NUL byte or a lone surrogate in the header value + # (``codecs.lookup`` raises it before it gets to the registry). + canonical = None + text = None + if canonical == "iso8859-1": + try: + # Strict UTF-8, tolerating a multi-byte sequence the slice above + # cut in half. Final when nothing was cut, so a real Latin-1 body + # ending in a lead byte falls back instead of losing its tail. + decoder = codecs.getincrementaldecoder("utf-8")() + text = decoder.decode(head, not truncated) + except UnicodeDecodeError: + text = None + if text is None: + try: + text = head.decode(codec, errors="replace") + except (LookupError, ValueError): # ValueError covers UnicodeError + text = head.decode("utf-8", errors="replace") + excerpt = _safe_body_excerpt(text, limit) + if truncated and not excerpt.endswith(_TRUNCATION_MARKER): + excerpt += _TRUNCATION_MARKER + return excerpt + + +def _required_records(data, key, what, response): + """Return ``data[key]`` as a list of JSON objects, or raise. + + ``_get_json`` vouches for the body being an object; this vouches for the + one key read out of it, so no ``KeyError`` escapes as the failure. + """ + records = data.get(key) + if not isinstance(records, list) or not all( + isinstance(record, Mapping) for record in records + ): + raise SecretServerError( + f"{what} did not return '{key}' as a list of objects", response + ) + return records + + +def _describe_response(response): + """Build a sanitized, length-capped error message from a response. + + Reads ``.content`` rather than ``.text``, which would decode and + charset-sniff the whole body to quote a couple of hundred characters. + """ + try: + content = response.content + except Exception as exc: + logger.debug( + "Could not read response body for an error message: %s", + type(exc).__name__, + ) + content = b"" + excerpt = _safe_body_excerpt_bytes( + content, encoding=getattr(response, "encoding", None) + ) + message = f"HTTP {response.status_code}" + return f"{message}: {excerpt}" if excerpt else message + + +def _validated_vault_url(url, response): + """Return ``(hostname, url)`` for an https vault URL, or raise. + + ``hostname`` rather than ``netloc``: ``https://@`` has a netloc but no + host, and would only fail later inside ``requests``. + """ + try: + parsed = urlsplit(url) if isinstance(url, str) else None + except ValueError: # unclosed IPv6 bracket, NFKC-changing netloc + parsed = None + if parsed is None or parsed.scheme != "https" or not parsed.hostname: + raise SecretServerError( + "Vault connection URL is not a valid https URL: " + f"{_safe_body_excerpt(repr(url))}", + response, + ) + return parsed.hostname, url.rstrip("/") + + +class _DetectionFlight: + """One in-progress server-type detection, shared by concurrent callers. + + The registering caller owns the probe; others wait on ``done``, then take + ``server_type`` or raise ``error``. ``superseded``: retired before it ended. + """ + + __slots__ = ("done", "server_type", "error", "superseded") + + def __init__(self): + self.done = Event() + self.server_type = None + self.error = None + self.superseded = False + + +class FileAttachment(bytes): + """The contents of a secret's file field, as the bytes the server sent. + + Keeps the ``requests.Response`` members a consumer of an earlier release + read -- ``.content``, ``.text``, ``.encoding`` -- and no other. + """ + + # Class-level defaults: pickle protocols 0 and 1 rebuild through + # ``copyreg._reconstructor``, not ``__new__``, so these keep ``.text`` + # working even if an instance is restored without its own attributes. + encoding = None + filename = None + + def __new__(cls, data, encoding=None, filename=None): + attachment = super().__new__(cls, data) + attachment.encoding = encoding + attachment.filename = filename + return attachment + + @property + def content(self): + """The attachment exactly as the server sent it, as plain ``bytes``.""" + return bytes(self) + + @property + def text(self): + """The attachment decoded as text, replacing undecodable bytes. + + A declared Latin-1 yields to valid UTF-8, because ``requests`` labels + every charset-less ``text/*`` body Latin-1. So does an unusable codec. + """ + codec = self.encoding if isinstance(self.encoding, str) else "" + try: + if codecs.lookup(codec or "utf-8").name == "iso8859-1": + return self.decode("utf-8") + except (LookupError, ValueError): # unusable codec, or not valid UTF-8 + pass + try: + return self.decode(codec or "utf-8", errors="replace") + except (LookupError, ValueError): # ValueError covers UnicodeError + return self.decode("utf-8", errors="replace") + + def __getnewargs__(self): + # Pins the round trip: ``bytes`` happens to supply this, but no rule + # of the model says so. The bytes are the only argument, so nothing + # re-runs a subclass's ``__init__``; the state dict carries the rest. + return (bytes(self),) + + def __repr__(self): + # Bounded on purpose: an attachment can be megabytes, and ``bytes``' + # own repr would put all of it into any log line holding a secret. + # ``filename`` is server data: sliced, then escaped and capped. + try: + name = self.filename + if not name: + name = "" + elif isinstance(name, (str, bytes)): + name = repr(name[: _FILENAME_EXCERPT_LIMIT + 1]) + else: + name = repr(name)[: _FILENAME_EXCERPT_LIMIT + 1] + except Exception: # only a hand-built filename can get here + name = "" + if name: + name = f" {_safe_body_excerpt(name, _FILENAME_EXCERPT_LIMIT)}" + return f"<{type(self).__name__}{name}: {len(self)} bytes>" + + def __str__(self): + # ``bytes`` defines ``__str__`` itself, so overriding only ``__repr__`` + # would leave ``print`` and f-strings dumping the whole attachment. + return repr(self) + @dataclass class ServerSecret: @@ -50,6 +347,8 @@ class Field: field_description: str field_name: str filename: str + # ``str`` for an ordinary field, a ``FileAttachment`` for a file field + # fetched with ``fetch_file_attachments``. value: str slug: str @@ -145,12 +444,61 @@ def __init__(self, **kwargs): setattr(self, k, v) +def _expires_in_seconds(value): + """``value`` as a finite float, or ``None`` when it is not a number. + + Booleans are not numbers here: ``True`` is not a one-second lifetime. + """ + if isinstance(value, bool): + return None + try: + seconds = float(value) + except (TypeError, ValueError): + return None + return seconds if math.isfinite(seconds) else None + + +def _with_validated_expires_in(grant, response): + """Return ``grant`` with a usable ``expires_in``, or raise. + + Missing or null defaults to ``_DEFAULT_GRANT_LIFETIME_SECONDS``; a value + that is not finite raises here. Zero is honoured, and ``_refresh`` warns. + """ + expires_in = grant.get("expires_in") + if expires_in is None: + logger.debug( + "Access grant carried no expires_in; assuming a %ss lifetime.", + _DEFAULT_GRANT_LIFETIME_SECONDS, + ) + return {**grant, "expires_in": _DEFAULT_GRANT_LIFETIME_SECONDS} + if _expires_in_seconds(expires_in) is None: + raise SecretServerError( + "Token endpoint returned a non-numeric expires_in: " + f"{_safe_body_excerpt(repr(expires_in))}", + response, + ) + return grant + + class SecretServerError(Exception): - """An Exception that includes a message and the server response""" + """An Exception that includes a message and the server response. + + ``message`` is always a string, never an object repr. ``.response`` is + in-memory only: :meth:`__reduce__` drops it so a pickle carries no secret. + """ def __init__(self, message, response=None, *args, **kwargs): self.message = message - super().__init__(*args, **kwargs) + self.response = response + # Pass message through so str(exception) is populated for default + # traceback/log output, not just the .message attribute. + super().__init__(message, *args, **kwargs) + + def __reduce__(self): + # Rebuild from the message alone, so ``response`` never reaches a pickle: + # it holds the PreparedRequest, whose body is the OAuth2 grant and whose + # headers carry the bearer token. Runtimes pickle exceptions unasked. + return (type(self), (self.message,)) class SecretServerClientError(SecretServerError): @@ -164,8 +512,107 @@ class SecretServerServiceError(SecretServerError): class Authorizer(ABC): """Main abstract base class for all Authorizer access methods.""" + # Accepted values for an explicit ``server_type`` override and for cached + # detections. + VALID_SERVER_TYPES = ("secret_server", "platform") + + # Bounded LRU mapping a normalized base_url to its detected server type, + # shared by every subclass so the probe pair fires once per URL per process. + # A caller with a process per lookup should pass an explicit ``server_type``. + _SERVER_TYPE_CACHE_MAXSIZE = 128 + _server_type_cache = OrderedDict() + _server_type_cache_lock = Lock() + # Detection probes currently in flight, keyed by normalized base_url. An + # entry exists only while its probe runs, so this is bounded by live + # concurrency rather than by the number of distinct URLs ever seen. + _server_type_flights = {} + + @classmethod + def _normalize_server_type(cls, server_type): + """Validate and normalize an explicit ``server_type`` value. + + :raise :class:`SecretServerError` when ``server_type`` is not one of + ``VALID_SERVER_TYPES``. + """ + normalized = str(server_type).strip().lower() + if normalized not in cls.VALID_SERVER_TYPES: + raise SecretServerError( + f"Invalid server_type {server_type!r}; expected one of " + f"{cls.VALID_SERVER_TYPES}." + ) + return normalized + + # Shared state below is addressed as ``Authorizer.*``, never ``cls.*``: + # there is one process-wide cache for every subclass. + @staticmethod + def _start_or_join_detection(key): + """Resolve ``key`` against the cache and the flight registry at once. + + Returns ``(cached, flight, is_leader)``; ``flight`` is ``None`` on a hit + and ``is_leader`` owns the probe. One acquisition closes the race. + """ + with Authorizer._server_type_cache_lock: + cache = Authorizer._server_type_cache + if key in cache: + cache.move_to_end(key) + return cache[key], None, False + flight = Authorizer._server_type_flights.get(key) + if flight is not None: + return None, flight, False + flight = _DetectionFlight() + Authorizer._server_type_flights[key] = flight + return None, flight, True + + @staticmethod + def _retire_flight(key, flight): + """Drop ``flight`` from the registry if it is still the one registered. + + Call with the cache lock held. False means a waiter that gave up on it, + or ``clear_server_type_cache``, already replaced or removed it. + """ + if Authorizer._server_type_flights.get(key) is flight: + del Authorizer._server_type_flights[key] + return True + return False + @staticmethod - def add_bearer_token_authorization_header(bearer_token, existing_headers={}): + def _finish_detection(key, flight, server_type, error): + """Publish a flight's outcome, cache a success, and retire the flight. + + Only the flight still registered may write the cache: a retired one is + stale, and last-write-wins would resurrect an answer already discarded. + """ + flight.server_type = server_type + flight.error = error + try: + with Authorizer._server_type_cache_lock: + current = Authorizer._retire_flight(key, flight) + if current and server_type is not None: + cache = Authorizer._server_type_cache + cache[key] = server_type + cache.move_to_end(key) + while len(cache) > Authorizer._SERVER_TYPE_CACHE_MAXSIZE: + cache.popitem(last=False) + finally: + # Waiters are released whatever happened above, or they would + # sit on the event until their own timeout. + flight.done.set() + + @staticmethod + def clear_server_type_cache(): + """Clear the process-scoped server-detection cache. + + Cached for the life of the process with no TTL, so this is the escape + hatch for a re-provisioned ``base_url``. Flights are dropped too. + """ + with Authorizer._server_type_cache_lock: + Authorizer._server_type_cache.clear() + for flight in Authorizer._server_type_flights.values(): + flight.superseded = True # its outcome no longer counts + Authorizer._server_type_flights.clear() + + @staticmethod + def add_bearer_token_authorization_header(bearer_token, existing_headers=None): """Adds an HTTP `Authorization` header containing the `Bearer` token :param existing_headers: a ``dict`` containing the existing headers @@ -176,47 +623,161 @@ def add_bearer_token_authorization_header(bearer_token, existing_headers={}): return { "Authorization": "Bearer " + bearer_token, - **existing_headers, + **(existing_headers or {}), } - def _perform_server_detection(self, base_url): - """Detects if the server is Secret Server or Platform by health check endpoints.""" - secret_server_endpoint = base_url.rstrip("/") + "/api/v1/healthcheck" - platform_endpoint = base_url.rstrip("/") + "/health" + def _perform_server_detection(self, base_url, server_type=None): + """Resolve whether the server is Secret Server or Platform. - if self._validate_health_endpoint(secret_server_endpoint): - self._server_type = "secret_server" - return - if self._validate_health_endpoint(platform_endpoint): - self._server_type = "platform" + An explicit ``server_type`` applies to this instance only: no probe, and + never cached, being unverified. Otherwise the probe pair runs once. + """ + if server_type is not None: + # Per-instance only; intentionally NOT seeded into the shared cache + # so an unverified override cannot poison auto-detection for others. + self._server_type = self._normalize_server_type(server_type) return + + self._server_type = self._detect_server_type_once(base_url.rstrip("/")) + + def _detect_server_type_once(self, key): + """Return the server type for ``key``, probing at most once per flight. + + Waiters take the leader's type, or raise their own copy of its error so + no traceback is rewritten. Past ``_DETECTION_WAIT_TIMEOUT`` they lead. + """ + while True: + cached, flight, is_leader = self._start_or_join_detection(key) + if cached is not None: + return cached + if is_leader: + return self._lead_detection(key, flight) + + if flight.done.wait(timeout=_DETECTION_WAIT_TIMEOUT): + if flight.error is None: + return flight.server_type + if flight.superseded: + # A clear or a takeover made this failure stale; the cache + # or the newer flight holds the current answer. + continue + raise self._shared_failure(flight.error) from flight.error + + logger.warning( + "Server-type detection for %s did not finish within %ss; " + "probing again from this thread.", + key, + _DETECTION_WAIT_TIMEOUT, + ) + with Authorizer._server_type_cache_lock: + if Authorizer._retire_flight(key, flight): + flight.superseded = True + + def _lead_detection(self, key, flight): + """Run the probe for a flight this caller registered, then publish it.""" + server_type = None + error = None + try: + server_type = self._probe_server_type(key) + return server_type + except Exception as exc: + error = exc + if isinstance(exc, SecretServerError): + raise + # Waiters receive ``_shared_failure(exc)``; the leader must not see a + # different type for the same failure just because it won the flight + # registration. Latent today: the probe swallows every Exception. + raise self._shared_failure(exc) from exc + except BaseException: + # KeyboardInterrupt and SystemExit belong to this thread alone. + # Waiters get an ordinary error they can handle, not a foreign + # interrupt raised in the middle of their own work. + error = SecretServerError( + "Server type detection was interrupted before it completed." + ) + raise + finally: + self._finish_detection(key, flight, server_type, error) + + @staticmethod + def _shared_failure(error): + """A waiter's own exception carrying the leader's failure.""" + if isinstance(error, SecretServerError): + try: + return type(error)(error.message, error.response) + except TypeError: + # A subclass with its own constructor still shares the failure, + # as the base type. + return SecretServerError(error.message, error.response) + return SecretServerError( + f"Server type detection failed: {type(error).__name__}" + ) + + def _probe_server_type(self, base_url): + """Probe the health-check endpoints and return the detected type. + + :raise :class:`SecretServerError` when neither endpoint reports a + healthy status. + """ + if self._validate_health_endpoint(_join_url(base_url, "/api/v1/healthcheck")): + return "secret_server" + if self._validate_health_endpoint(_join_url(base_url, "/health")): + return "platform" raise SecretServerError( "Unable to detect server type via health check endpoints." ) def _validate_health_endpoint(self, url): - """Validates if an endpoint returns healthy status.""" + """Validates if an endpoint returns healthy status. + + Requires a 2xx and one of the two shapes the products emit: ``Healthy`` + true in a JSON object, or a body that is exactly ``healthy``. + """ try: - response = requests.get(url, timeout=60) - except Exception: + response = requests.get(url, timeout=DEFAULT_REQUEST_TIMEOUT) + except Exception as exc: + logger.debug("Health probe to %s failed: %s", url, type(exc).__name__) + return False + + # Explicit 2xx: ``response.ok`` is true for anything under 400, which + # would admit a 3xx a proxy answered with a healthy-looking body. + if not 200 <= response.status_code < 300: return False try: - response_body = response.content - except Exception: + return self._body_reports_healthy(response) + except Exception as exc: + # An unreadable body means "not healthy, try the next endpoint", + # never "abort detection". The helper's narrow ``ValueError`` catch + # is for the JSON parse; anything else must not end detection here. + logger.debug( + "Health body from %s was unreadable: %s", url, type(exc).__name__ + ) return False + @staticmethod + def _body_reports_healthy(response): + """Whether a 2xx health-check body reports a healthy server. + + A JSON object whose ``Healthy`` is boolean ``true`` (Secret Server), or + a body that is exactly ``healthy`` (Platform). Anything else is not. + """ try: json_data = response.json() - return json_data.get("Healthy", False) - except Exception: - return b"Healthy" in response_body or b"healthy" in response_body + except ValueError: + json_data = None + + if isinstance(json_data, Mapping): + return json_data.get("Healthy") is True + if json_data is not None: + return False + + return response.text.strip().lower() == "healthy" @abstractmethod def get_access_token(self): """Returns the access_token from a Grant Request""" - def headers(self, existing_headers={}): + def headers(self, existing_headers=None): """Returns a dictionary containing headers for REST API calls""" return self.add_bearer_token_authorization_header( self.get_access_token(), existing_headers @@ -231,10 +792,42 @@ class AccessTokenAuthorizer(Authorizer): def get_access_token(self): return self.access_token - def __init__(self, access_token, base_url): + # Same policy as PasswordGrantAuthorizer: a pickle leaves the process and + # this holds a live bearer token. ``copy`` shares the reduce protocol, so + # refusing that alone would break copy/deepcopy; both are defined below. + + def __copy__(self): + clone = self.__class__.__new__(self.__class__) + clone.__dict__.update(self.__dict__) + return clone + + def __deepcopy__(self, memo): + clone = self.__class__.__new__(self.__class__) + memo[id(self)] = clone + clone.__dict__.update(copy.deepcopy(self.__dict__, memo)) + return clone + + def __reduce__(self): + raise TypeError( + f"{self.__class__.__name__} holds a live bearer token and cannot be " + "pickled. Construct one from configuration in the target process " + "instead; use copy.deepcopy() for an in-memory copy." + ) + + def __init__(self, access_token, base_url, server_type=None): + """ + :param server_type: optionally ``"secret_server"`` or ``"platform"`` to + skip health-check detection entirely (no probe is issued). + """ self.access_token = access_token self.base_url = base_url.rstrip("/") - self._perform_server_detection(self.base_url) + _warn_if_insecure(self.base_url) + if server_type is None: + # No keyword, so a subclass that still overrides the original + # one-argument hook keeps working. + self._perform_server_detection(self.base_url) + else: + self._perform_server_detection(self.base_url, server_type=server_type) class PasswordGrantAuthorizer(Authorizer): @@ -254,76 +847,199 @@ def get_access_grant(token_url, grant_request): other than a valid Access Grant """ - response = requests.post(token_url, grant_request, timeout=60) + response = requests.post( + token_url, grant_request, timeout=DEFAULT_REQUEST_TIMEOUT + ) try: # TSS returns a 200 (OK) containing HTML for some error conditions - return json.loads(SecretServer.process(response).content) - except json.JSONDecodeError: - raise SecretServerError(response) + # ``or b""``: ``.content`` is None when ``raw`` is, and + # ``json.loads`` answers that with TypeError, not ValueError. + grant = json.loads(SecretServer.process(response).content or b"") + except ValueError: + raise SecretServerError( + "Token endpoint did not return a JSON access grant " + f"({_describe_response(response)})", + response, + ) + + # A 200 can also carry a JSON *error* body, or JSON that is not an object. + # Reject those here, quoting the server's own explanation, rather than + # storing them and failing later with a KeyError in get_access_token(). + token = grant.get("access_token") if isinstance(grant, Mapping) else None + if not isinstance(token, str) or not token: + detail = None + if isinstance(grant, Mapping): + detail = grant.get("error_description") or grant.get("error") + if isinstance(detail, str) and detail: + detail = _safe_body_excerpt(detail) + else: + detail = _describe_response(response) # already capped + raise SecretServerError( + f"Token endpoint did not return an access grant: {detail}", + response, + ) + return _with_validated_expires_in(grant, response) + + def _grant_is_fresh(self, seconds_of_drift): + """Whether the stored grant can be used without a token request. + + Safe to call unlocked: a half-written pair, a naive timestamp (the + pre-2.1 convention) or a non-datetime one simply reads as stale. + """ + grant = getattr(self, "access_grant", None) + refreshed = getattr(self, "access_grant_refreshed", None) + if grant is None or getattr(refreshed, "tzinfo", None) is None: + return False + validity = self._grant_validity_seconds(grant, seconds_of_drift) + return refreshed + timedelta(seconds=validity) > datetime.now(timezone.utc) def _refresh(self, seconds_of_drift=300): - """Refreshes the *OAuth2 Access Grant* if it has expired or will in the next - `seconds_of_drift` seconds. + """Refresh the *OAuth2 Access Grant* if it expires within `seconds_of_drift`. + + A fresh grant is used without taking ``_refresh_lock``, so callers are + never stalled behind another thread's token request; one refresher. :raise :class:`SecretServerError` when the server returns anything other than a valid Access Grant """ - - if ( - hasattr(self, "access_grant") - and self.access_grant_refreshed - + timedelta(seconds=self.access_grant["expires_in"] + seconds_of_drift) - > datetime.now() - ): + if self._grant_is_fresh(seconds_of_drift): return - else: - # Detect server type if not already done + + with self._refresh_lock: + if self._grant_is_fresh(seconds_of_drift): + return # another thread refreshed while we waited + + # Detect the server type if not already resolved. if not hasattr(self, "_server_type"): self._perform_server_detection(self.base_url) - # Decide token_path_uri if not provided + + # Decide token_path_uri if not provided. if not self.token_path_uri: - if self._server_type == "secret_server": - self.token_path_uri = self.TOKEN_PATH_URI - elif self._server_type == "platform": - self.token_path_uri = self.PLATFORM_TOKEN_PATH_URI - else: - raise SecretServerError("Unknown server type for token request.") - if self._server_type == "secret_server": - self.token_url = ( - self.base_url.rstrip("/") + "/" + self.token_path_uri.strip("/") + # Resolved through ``self`` so a subclass that overrides either + # constant -- the pre-existing extension point -- is honoured. + self.token_path_uri = ( + self.PLATFORM_TOKEN_PATH_URI + if self._server_type == "platform" + else self.TOKEN_PATH_URI ) + + self.token_url = _join_url(self.base_url, self.token_path_uri) + + if self._server_type == "secret_server": grant_request = { "username": self.username, "password": self.password, "grant_type": "password", } - if hasattr(self, "domain") and self.domain: + if self.domain: grant_request["domain"] = self.domain - self.access_grant = self.get_access_grant(self.token_url, grant_request) - self.access_grant_refreshed = datetime.now() - elif self._server_type == "platform": - self.token_url = ( - self.base_url.rstrip("/") + "/" + self.token_path_uri.strip("/") - ) + else: grant_request = { "client_id": self.username, "client_secret": self.password, "grant_type": "client_credentials", "scope": "xpmheadless", } - self.access_grant = self.get_access_grant(self.token_url, grant_request) - self.access_grant_refreshed = datetime.now() - else: - raise SecretServerError("Unknown server type for token request.") - def __init__(self, base_url, username, password, token_path_uri=None, domain=None): + grant = self.get_access_grant(self.token_url, grant_request) + lifetime = _expires_in_seconds(grant.get("expires_in")) + if ( + lifetime is not None + and lifetime < 1 + and not self._short_lifetime_warned + ): + # Once per authorizer, not once per call: with no reuse window + # every API call is a token request, and a warning per call + # would flood the log with the same message. + self._short_lifetime_warned = True + logger.warning( + "Access grant expires_in is %s; with no reuse window the token " + "will be re-requested on every API call until the server sends " + "a lifetime of at least one second.", + _safe_body_excerpt(repr(grant.get("expires_in"))), + ) + # Ordinary assignments, so a subclass property or slot still works; + # grant first, timestamp second, because ``_copy_with_fresh_lock`` + # snapshots unlocked and must not pair a stale grant with a new stamp. + self.access_grant = grant + self.access_grant_refreshed = datetime.now(timezone.utc) + + def __init__( + self, + base_url, + username, + password, + token_path_uri=None, + domain=None, + server_type=None, + ): + """ + :param server_type: optionally ``"secret_server"`` or ``"platform"`` to + skip health-check detection entirely (no probe is issued); the + matching token endpoint is selected without probing. + """ self.base_url = base_url.rstrip("/") + _warn_if_insecure(self.base_url) self.username = username self.password = password self.domain = domain self.token_path_uri = token_path_uri # May be None, will decide in _refresh self.token_url = None - self.grant_request = None + self._short_lifetime_warned = False + self._refresh_lock = Lock() + # When an explicit type is given, resolve it now (no network) so the + # lazy detection in _refresh is skipped and no probe is ever issued. + if server_type is not None: + self._perform_server_detection(self.base_url, server_type=server_type) + + @staticmethod + def _grant_validity_seconds(access_grant, seconds_of_drift): + """Seconds a grant is reused before a proactive refresh. + + ``seconds_of_drift`` before expiry, never sooner than half the lifetime. + The non-numeric fallbacks matter only for a grant assigned by hand. + """ + expires_in = _expires_in_seconds( + access_grant.get("expires_in", _DEFAULT_GRANT_LIFETIME_SECONDS) + ) + if expires_in is None or expires_in <= 0: + return 0.0 + expires_in = min(expires_in, _MAX_GRANT_LIFETIME_SECONDS) + return max(expires_in - seconds_of_drift, expires_in / 2) + + # Copying is supported, serialization is refused -- deliberately. ``copy`` + # hands out an independent authorizer with its own refresh lock; a pickle + # would write the plaintext password and bearer token wherever it goes. + + def _copy_with_fresh_lock(self, deep, memo=None): + clone = self.__class__.__new__(self.__class__) + if memo is not None: + memo[id(self)] = clone + # Snapshot WITHOUT ``_refresh_lock``: taking it would deadlock a copy + # made from code already holding it. ``dict()`` cannot tear, but can + # land mid-publish, so an incomplete grant pair is dropped below. + state = dict(self.__dict__) + state.pop("_refresh_lock", None) + if ("access_grant" in state) != ("access_grant_refreshed" in state): + state.pop("access_grant", None) + state.pop("access_grant_refreshed", None) + for name, value in state.items(): + clone.__dict__[name] = copy.deepcopy(value, memo) if deep else value + clone._refresh_lock = Lock() + return clone + + def __copy__(self): + return self._copy_with_fresh_lock(deep=False) + + def __deepcopy__(self, memo): + return self._copy_with_fresh_lock(deep=True, memo=memo) + + def __reduce__(self): + raise TypeError( + f"{self.__class__.__name__} holds live credentials and cannot be " + "pickled. Construct one from configuration in the target process " + "instead; use copy.deepcopy() for an in-memory copy." + ) def get_access_token(self): self._refresh() @@ -340,9 +1056,15 @@ def __init__( domain, password, token_path_uri=None, + server_type=None, ): super().__init__( - base_url, username, password, token_path_uri=token_path_uri, domain=domain + base_url, + username, + password, + token_path_uri=token_path_uri, + domain=domain, + server_type=server_type, ) @@ -372,17 +1094,26 @@ def process(response): if response.status_code >= 200 and response.status_code < 300: return response if response.status_code >= 400 and response.status_code < 500: + # Fallback used when the body is JSON but carries no recognized + # message/error key, or is JSON that is not an object at all + # (``null``, a number, a string, a list). + message = f"HTTP {response.status_code}" try: - content = json.loads(response.content) - if "message" in content: - message = content["message"] - elif "error" in content and isinstance(content["error"], str): - message = content["error"] - except json.JSONDecodeError as err: - message = err.msg + content = json.loads(response.content or b"") + except ValueError: + # Keep the status and a sanitized body hint. The JSON parser's + # own complaint gave messages like "Expecting value", dropping + # the status code and any clue about what the server returned. + message = _describe_response(response) + else: + if isinstance(content, Mapping): + if isinstance(content.get("message"), str): + message = content["message"] + elif isinstance(content.get("error"), str): + message = content["error"] raise SecretServerClientError(message, response) else: - raise SecretServerServiceError(response) + raise SecretServerServiceError(_describe_response(response), response) def headers(self): """Returns a dictionary containing HTTP headers.""" @@ -395,7 +1126,7 @@ def __init__( api_path_uri=API_PATH_URI, ): """ - :param base_url: The base URL e.g. ``http://localhost/SecretServer`` + :param base_url: The base URL e.g. ``https://localhost/SecretServer`` :type base_url: str :param authorizer: The authorization method to be used :type authorizer: Authorizer @@ -403,45 +1134,117 @@ def __init__( :type api_path_uri: str """ self.base_url = base_url.rstrip("/") + # An authorizer built for this same URL already warned; a second + # identical warning only ever shows up under ``-W always``. + if getattr(authorizer, "base_url", None) != self.base_url: + _warn_if_insecure(self.base_url) self.platform_url = self.base_url self.authorizer = authorizer self._api_path_uri = api_path_uri + self._vault_url_fetched = False @property def api_url(self): - return f"{self.base_url}/{self._api_path_uri.strip('/')}" + return _join_url(self.base_url, self._api_path_uri) def ensure_vault_url(self): - """For platform, fetch and set the vault URL before making API calls.""" - # Only needed for platform scenario - if ( - hasattr(self.authorizer, "_server_type") - and self.authorizer._server_type == "platform" - ): - if not hasattr(self, "_vault_url_fetched") or not self._vault_url_fetched: - access_token = self.authorizer.get_access_token() - vaults_endpoint = self.platform_url + "/vaultbroker/api/vaults" - headers = {"Authorization": f"Bearer {access_token}"} - resp = requests.get(vaults_endpoint, headers=headers, timeout=60) - if resp.status_code != 200: - raise SecretServerError( - f"Failed to fetch vault details: HTTP {resp.status_code} - {resp.text}" - ) - try: - data = resp.json() - except Exception as ex: - raise SecretServerError(f"Failed to parse vault details: {ex}") - for vault in data.get("vaults", []): - if vault.get("isDefault") and vault.get("isActive"): - conn = vault.get("connection", {}) - url = conn.get("url") - if url: - self.base_url = url.rstrip("/") - self._vault_url_fetched = True - return - raise SecretServerError( - "No configured default and active vault found in vault details." - ) + """For platform, fetch and set the vault URL before making API calls. + + Safe in any order relative to :meth:`headers`, which resolves the token + and so makes a lazy authorizer learn its type. Remembered per instance. + """ + if self._vault_url_fetched: + return + + headers = None + server_type = getattr(self.authorizer, "_server_type", None) + if server_type is None: + # A lazily detected authorizer learns its type while resolving the + # token; resolve it once here rather than again in ``_get``. + headers = self.headers() + server_type = getattr(self.authorizer, "_server_type", None) + if server_type != "platform": + # Secret Server is addressed at base_url directly; nothing to switch. + self._vault_url_fetched = True + return + if headers is None: + headers = self.headers() + + vaults_endpoint = _join_url(self.platform_url, "/vaultbroker/api/vaults") + resp = requests.get( + vaults_endpoint, headers=headers, timeout=DEFAULT_REQUEST_TIMEOUT + ) + if resp.status_code != 200: + raise SecretServerError( + f"Failed to fetch vault details: {_describe_response(resp)}", resp + ) + try: + data = resp.json() + except Exception as ex: + raise SecretServerError(f"Failed to parse vault details: {ex}", resp) + vaults = data.get("vaults") if isinstance(data, Mapping) else None + if not isinstance(vaults, list): + raise SecretServerError( + f"Vault details did not contain a 'vaults' list: {_describe_response(resp)}", + resp, + ) + for vault in vaults: + if not isinstance(vault, Mapping): + continue + if not (vault.get("isDefault") and vault.get("isActive")): + continue + conn = vault.get("connection") + url = conn.get("url") if isinstance(conn, Mapping) else None + if not url: + continue + hostname, vault_url = _validated_vault_url(url, resp) + # ``hostname`` rather than the URL: userinfo must not reach the log. + logger.info( + "Switching base_url to platform vault connection URL at %s", hostname + ) + self.base_url = vault_url + self._vault_url_fetched = True + return + raise SecretServerError( + "No configured default and active vault found in vault details." + ) + + def _get(self, path, params=None): + """Issue an authenticated ``GET`` for ``path`` under :attr:`api_url`. + + The single owner of the read contract: vault switch, headers, timeout + and :meth:`process`. ``params`` takes any form ``requests`` accepts. + """ + self.ensure_vault_url() + return self.process( + requests.get( + _join_url(self.api_url, path), + params=params, + headers=self.headers(), + timeout=DEFAULT_REQUEST_TIMEOUT, + ) + ) + + def _get_json(self, path, what, params=None, *, redact_body=False): + """``_get`` plus JSON parsing; returns ``(data, response)``. + + A body that is not a JSON object raises :class:`SecretServerError` + naming ``what``, response attached, body excerpted unless ``redact_body``. + """ + response = self._get(path, params=params) + try: + data = json.loads(response.content or b"") + except ValueError: + problem = "did not return JSON" + else: + if isinstance(data, Mapping): + return data, response + problem = "did not return a JSON object" + if redact_body: # the body may be secret; the status never is + detail = f": HTTP {response.status_code}" + else: + detail = f": {_describe_response(response)}" + raise SecretServerError(f"{what} {problem}{detail}", response) def get_secret_json(self, id, query_params=None): """Gets a Secret from Secret Server @@ -457,23 +1260,7 @@ def get_secret_json(self, id, query_params=None): :raise: :class:`SecretServerError` when the REST API call fails for any other reason """ - headers = self.headers() - self.ensure_vault_url() - endpoint_url = f"{self.api_url}/secrets/{id}" - - if query_params is None: - return self.process( - requests.get(endpoint_url, headers=headers, timeout=60) - ).text - else: - return self.process( - requests.get( - endpoint_url, - params=query_params, - headers=headers, - timeout=60, - ) - ).text + return self._get(f"/secrets/{id}", params=query_params).text def get_folder_json(self, id, query_params=None, get_all_children=True): """Gets a Folder from Secret Server @@ -489,23 +1276,11 @@ def get_folder_json(self, id, query_params=None, get_all_children=True): :raise: :class:`SecretServerError` when the REST API call fails for any other reason """ - headers = self.headers() - self.ensure_vault_url() - endpoint_url = f"{self.api_url}/folders/{id}" - if get_all_children: - query_params["getAllChildren"] = "true" - - if query_params is None: - return self.process(requests.get(endpoint_url, headers=headers)).text - else: - return self.process( - requests.get( - endpoint_url, - params=query_params, - headers=headers, - ) - ).text + # A copy of the caller's params with the flag sent once and winning, + # in whatever form ``requests`` accepts. + query_params = _with_query_flag(query_params, "getAllChildren", "true") + return self._get(f"/folders/{id}", params=query_params).text def get_secret(self, id, fetch_file_attachments=True, query_params=None): """Gets a secret @@ -526,32 +1301,35 @@ def get_secret(self, id, fetch_file_attachments=True, query_params=None): any other reason """ - response = self.get_secret_json(id, query_params=query_params) - - try: - secret = json.loads(response) - except json.JSONDecodeError: - raise SecretServerError(response) + # The secrets endpoint: never echo its body into an error message, + # since it may contain secret field values. + secret, secret_response = self._get_json( + f"/secrets/{id}", "Secret endpoint", params=query_params, redact_body=True + ) if fetch_file_attachments: - for item in secret["items"]: - if item["fileAttachmentId"]: - endpoint_url = f"{self.api_url}/secrets/{id}/fields/{item['slug']}" - if query_params is None: - item["itemValue"] = self.process( - requests.get( - endpoint_url, headers=self.headers(), timeout=60 - ) - ) - else: - item["itemValue"] = self.process( - requests.get( - endpoint_url, - params=query_params, - headers=self.headers(), - timeout=60, - ) + # Each attachment goes through _get, which rebuilds headers: a lock + # and a comparison unless a refresh is due. Refreshing mid-burst + # beats sending the rest an expired token and failing them with 401. + items = _required_records( + secret, "items", "Secret endpoint", secret_response + ) + for item in items: + if item.get("fileAttachmentId"): + slug = item.get("slug") + if not isinstance(slug, str) or not slug: + raise SecretServerError( + "Secret endpoint returned a file field with no 'slug'", + secret_response, ) + response = self._get( + f"/secrets/{id}/fields/{slug}", params=query_params + ) + item["itemValue"] = FileAttachment( + response.content or b"", + encoding=getattr(response, "encoding", None), + filename=item.get("filename"), + ) return secret def get_folder(self, id, query_params=None, get_all_children=False): @@ -571,15 +1349,11 @@ def get_folder(self, id, query_params=None, get_all_children=False): any other reason """ - response = self.get_folder_json( - id, query_params=query_params, get_all_children=get_all_children + if get_all_children: + query_params = _with_query_flag(query_params, "getAllChildren", "true") + folder, _ = self._get_json( + f"/folders/{id}", "Folder endpoint", params=query_params ) - - try: - folder = json.loads(response) - except json.JSONDecodeError: - raise SecretServerError(response) - return folder def get_secret_by_path(self, secret_path, fetch_file_attachments=True): @@ -632,23 +1406,7 @@ def search_secrets(self, query_params=None): :raise: :class:`SecretServerError` when the REST API call fails for any other reason """ - headers = self.headers() - self.ensure_vault_url() - endpoint_url = f"{self.api_url}/secrets" - - if query_params is None: - return self.process( - requests.get(endpoint_url, headers=headers, timeout=60) - ).text - else: - return self.process( - requests.get( - endpoint_url, - params=query_params, - headers=headers, - timeout=60, - ) - ).text + return self._get("/secrets", params=query_params).text def lookup_folders(self, query_params=None): """Lookup Folders from Secret Server @@ -662,20 +1420,7 @@ def lookup_folders(self, query_params=None): :raise: :class:`SecretServerError` when the REST API call fails for any other reason """ - headers = self.headers() - self.ensure_vault_url() - endpoint_url = f"{self.api_url}/folders/lookup" - - if query_params is None: - return self.process(requests.get(endpoint_url, headers=headers)).text - else: - return self.process( - requests.get( - endpoint_url, - params=query_params, - headers=headers, - ) - ).text + return self._get("/folders/lookup", params=query_params).text def get_secret_ids_by_folderid(self, folder_id): """Gets a list of secrets ids by folder_id @@ -689,22 +1434,20 @@ def get_secret_ids_by_folderid(self, folder_id): :raise: :class:`SecretServerError` when the REST API call fails for any other reason """ - headers = self.headers() - self.ensure_vault_url() params = {"filter.folderId": folder_id} - endpoint_url = f"{self.api_url}/secrets/search-total" - params["take"] = self.process( - requests.get(endpoint_url, params=params, headers=headers, timeout=60) - ).text - response = self.search_secrets(query_params=params) - + total_response = self._get("/secrets/search-total", params=params) try: - secrets = json.loads(response) - except json.JSONDecodeError: - raise SecretServerError(response) + params["take"] = int(total_response.text) + except ValueError: + raise SecretServerError( + f"Unexpected non-numeric secrets count from search-total: " + f"{_safe_body_excerpt(total_response.text)}", + total_response, + ) + secrets, response = self._get_json("/secrets", "Secrets search", params=params) secret_ids = [] - for secret in secrets["records"]: + for secret in _required_records(secrets, "records", "Secrets search", response): secret_ids.append(secret["id"]) return secret_ids @@ -720,34 +1463,30 @@ def get_child_folder_ids_by_folderid(self, folder_id): :raise: :class:`SecretServerError` when the REST API call fails for any other reason """ - headers = self.headers() - self.ensure_vault_url() params = { "filter.parentFolderId": folder_id, "filter.limitToDirectDescendents": True, } params["take"] = 1 - endpoint_url = f"{self.api_url}/folders/lookup" - - params["take"] = self.process( - requests.get(endpoint_url, params=params, headers=headers) - ).json()["total"] - # Handle result of zero child folders - if params["take"] != 0: - response = self.lookup_folders(query_params=params) - - try: - response = json.loads(response) - except json.JSONDecodeError: - raise SecretServerError(response) - child_folder_ids = [] - for childFolder in response["records"]: - child_folder_ids.append(childFolder["id"]) - - return child_folder_ids - else: + lookup, lookup_response = self._get_json( + "/folders/lookup", "Folder lookup", params=params + ) + total = lookup.get("total") + if isinstance(total, bool) or not isinstance(total, int): + raise SecretServerError( + "Folder lookup did not return an integer 'total': " + f"{_safe_body_excerpt(repr(total))}", + lookup_response, + ) + if total == 0: return [] + params["take"] = total + page, response = self._get_json( + "/folders/lookup", "Folder lookup", params=params + ) + records = _required_records(page, "records", "Folder lookup", response) + return [child_folder["id"] for child_folder in records] class SecretServerV0(SecretServer): @@ -768,10 +1507,17 @@ def __init__( password, api_path_uri=SecretServer.API_PATH_URI, token_path_uri=None, + server_type=None, ): + """ + :param server_type: optionally ``"secret_server"`` or ``"platform"`` to + skip health-check detection, as on the authorizers. + """ super().__init__( base_url, - PasswordGrantAuthorizer(f"{base_url}", username, password, token_path_uri), + PasswordGrantAuthorizer( + base_url, username, password, token_path_uri, server_type=server_type + ), api_path_uri, ) diff --git a/example.py b/example.py index d37d7cc..3701a5d 100644 --- a/example.py +++ b/example.py @@ -23,10 +23,14 @@ try: secret = secret_server_cloud.get_secret(os.getenv("TSS_SECRET_ID")) serverSecret = ServerSecret(**secret) - print( - f"""username: {serverSecret.fields['username'].value} - password: {serverSecret.fields['password'].value} - template: {serverSecret.secret_template_name}""" - ) + # Never print secret values; mask them in any console/log output. + print(f"""username: {serverSecret.fields['username'].value} + password: ******** + template: {serverSecret.secret_template_name}""") except SecretServerError as error: - print(error.response.text) + # ``.response`` is None for errors raised before or without an HTTP + # response (e.g. server-type detection failure); ``.message`` is + # always populated and already excludes any full response body. + print(error.message) + if error.response is not None: + print(f"HTTP {error.response.status_code}") diff --git a/pyproject.toml b/pyproject.toml index 737c6fa..ebe463c 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -9,14 +9,29 @@ author-email = "GitHub@delinea.com" classifiers = [ "License :: OSI Approved :: Apache Software License", "Operating System :: OS Independent", - "Programming Language :: Python :: 3.8", - "Programming Language :: Python :: 3.9", "Programming Language :: Python :: 3.10", - "Programming Language :: Python :: 3.11" + "Programming Language :: Python :: 3.11", + "Programming Language :: Python :: 3.12" ] description-file = "README.md" +# BREAKING (consumer-facing): the requests floor was raised from 2.12.5 to 2.34.2 +# to clear CVE-2026-25645 (requests) and its transitive urllib3 advisories for +# downstream installs, not just CI. requests 2.34.2 requires Python >= 3.10. +# +# urllib3 and idna arrive transitively through requests, whose own floors are +# far lower (urllib3 >= 1.21.1). Without the floors below, a downstream +# `pip install python-tss-sdk` can still resolve exactly the versions the CVE +# pins in requirements.txt exist to exclude -- so the remediation would cover +# this repo's CI but never reach the published artifact. Floors (not ==) so +# consumers stay free to take newer fixed releases. requires = [ - "requests >= 2.12.5" + "requests >= 2.34.2", + "urllib3 >= 2.7.0", + "idna >= 3.18" ] -requires-python=">=3.8" +# BREAKING (consumer-facing): minimum Python raised from 3.8 to 3.10. The fixed +# requests/urllib3 releases that clear the flagged CVEs dropped 3.8/3.9 support +# (both EOL). Consumers on Python 3.8/3.9 must stay on an older SDK release or +# upgrade their runtime. See work item 741117. +requires-python=">=3.10" dist-name = "python-tss-sdk" diff --git a/requirements-dev.txt b/requirements-dev.txt new file mode 100644 index 0000000..51beb3c --- /dev/null +++ b/requirements-dev.txt @@ -0,0 +1,20 @@ +# Development/build/test tooling for this repo (not part of the SDK's runtime +# dependency surface). Layered so a test virtualenv installs only what it +# needs: requirements.txt (runtime pins) -> requirements-test.txt (test deps) +# -> this file (build and lint toolchain). +-r requirements-test.txt + +tox +flit +black==26.5.1 # pinned to address CVE-2026-32274 (directory traversal) and CVE-2024-21503 (ReDoS) +zipp==3.23.0 # not directly required, pinned by Snyk to avoid a vulnerability +filelock==3.32.0 # not directly required (transitive via tox), pinned to address CVE-2026-22701 and CVE-2025-68146 + +# pip is deliberately NOT pinned here. `pip install -r` cannot replace the pip +# that is running the install -- on Windows it fails outright with "Access is +# denied" -- so the upgrade has to happen in the outer interpreter instead: +# +# python -m pip install --upgrade "pip>=26.2" +# +# release.yml, run_tests.yml and the README setup steps all do exactly that, +# covering CVE-2026-8643, CVE-2026-6357, CVE-2026-13346 and CVE-2026-3219. diff --git a/requirements-test.txt b/requirements-test.txt new file mode 100644 index 0000000..8092066 --- /dev/null +++ b/requirements-test.txt @@ -0,0 +1,12 @@ +# Test-only dependencies for the offline and live suites. Inherits the runtime +# pins so tests exercise the exact requests/urllib3/idna versions consumers get +# from `pip install python-tss-sdk`, rather than floating "latest". +# +# Deliberately excludes the build and lint toolchain (tox, flit, black). tox +# installs this file into every test virtualenv, and each of those tools is +# installed by the workflow that actually uses it: run_tests.yml installs tox in +# the outer interpreter, lint.yml pins black, release.yml pins flit. +-r requirements.txt + +pytest +python-dotenv==1.2.2 # pinned to address CVE-2026-28684 (symlink attack in set_key/unset_key) diff --git a/requirements.txt b/requirements.txt index 3f6a39b..9cd8c64 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,8 +1,3 @@ -requests==2.32.4 -tox -pytest -python-dotenv -flit -black -urllib3==2.6.3 # not directly required, pinned by Snyk to avoid a vulnerability -zipp==3.23.0 # not directly required, pinned by Snyk to avoid a vulnerability +requests==2.34.2 # pinned to address CVE-2026-25645 (2.33.0 was never published) +urllib3==2.7.0 # not directly required, pinned by Snyk to avoid a vulnerability +idna==3.18 # not directly required (transitive via requests), pinned to address CVE-2026-45409 diff --git a/tests/conftest.py b/tests/conftest.py new file mode 100644 index 0000000..81a9078 --- /dev/null +++ b/tests/conftest.py @@ -0,0 +1,47 @@ +"""Fixtures shared by the offline test modules in this directory. + +Additive to the repository-root ``conftest.py``, which holds the live-tenant +fixtures. Neither is ``autouse``; offline modules opt in via ``pytestmark``. +""" + +import pytest + +from delinea.secrets.server import Authorizer +from fakes import HTTP_GET, HTTP_POST + + +@pytest.fixture +def clear_detection_cache(): + """Isolate the process-global server-detection cache. + + It lives on the ``Authorizer`` class for the life of the process, so + without this one test's cached detection changes what a later one runs. + """ + Authorizer.clear_server_type_cache() + yield + Authorizer.clear_server_type_cache() + + +@pytest.fixture +def no_network(monkeypatch): + """Turn an unmocked HTTP call in an offline test into a loud failure. + + Raising is not enough on its own: the health probe swallows exceptions, so + every attempt is recorded and asserted at teardown instead. + """ + attempts = [] + + def blocked(*args, **kwargs): + attempts.append(args[0] if args else kwargs.get("url")) + raise AssertionError( + "offline test attempted a real network call; patch " + "delinea.secrets.server.requests.get / .post in the test" + ) + + monkeypatch.setattr(HTTP_GET, blocked) + monkeypatch.setattr(HTTP_POST, blocked) + yield attempts + assert not attempts, ( + f"offline test reached the network guard {len(attempts)} time(s) and " + f"the SDK swallowed the failure: {attempts[:3]}" + ) diff --git a/tests/fakes.py b/tests/fakes.py new file mode 100644 index 0000000..289e758 --- /dev/null +++ b/tests/fakes.py @@ -0,0 +1,185 @@ +"""Shared test doubles for the offline test modules in this directory. + +Plain helpers, kept out of ``conftest.py`` so a second importable module of +that name cannot make imports depend on ``sys.path`` order. +""" + +import json +import time + +from delinea.secrets.server import ( + AccessTokenAuthorizer, + PasswordGrantAuthorizer, + SecretServer, +) + +# The two network primitives the SDK calls; patch these, never the literal. +HTTP_GET = "delinea.secrets.server.requests.get" +HTTP_POST = "delinea.secrets.server.requests.post" + +# Pass as ``json_data`` for a body that is the JSON literal ``null``: a real +# ``requests.Response`` returns ``None`` from ``json()`` for it, which is a +# different branch from "no JSON at all" (``json()`` raising). +JSON_NULL = object() + + +class FakeResponse: + """Minimal stand-in for ``requests.Response`` as consumed by the SDK. + + Exposes only what the SDK reads. ``json()`` raises ``ValueError`` when no + body was given; pass ``json_data=JSON_NULL`` for a body of ``null``. + """ + + def __init__(self, status_code=200, json_data=None, text=None): + self.status_code = status_code + # Mirrors ``requests.Response.ok``: true for anything under 400, so a + # test cannot pass here while production treats a 3xx differently. + self.ok = status_code < 400 + self._has_json = json_data is not None + self._json = None if json_data is JSON_NULL else json_data + if text is not None: + self.text = text + elif self._has_json: + self.text = json.dumps(self._json) + else: + self.text = "" + self.content = self.text.encode() + + def json(self): + if not self._has_json: + raise ValueError("no JSON body") + return self._json + + +class HostileBody: + """A 2xx response whose body cannot be read at all. + + ``json()`` and ``text`` raise something other than ``ValueError``, the case + the health-check guard exists for: "unhealthy", never "abort detection". + """ + + status_code = 200 + ok = True + + def json(self): + raise AttributeError("body accessor blew up") + + @property + def text(self): + raise AttributeError("body accessor blew up") + + +class BytesOnlyResponse: + """A response whose body can only be read as bytes. + + Reading ``.text`` makes ``requests`` decode (and charset-sniff) the whole + body, which the error path must not do just to keep a short excerpt. + """ + + status_code = 502 + ok = False + + def __init__(self, content): + self.content = content + + @property + def text(self): + raise AssertionError("the error path must not decode the whole body") + + def json(self): + raise ValueError("no JSON body") + + +class AttachmentResponse: + """The secret-field endpoint as ``requests`` delivers an attachment. + + ``.text`` raises, so a production path that decodes the file fails here + instead of quietly passing on a fake's empty string. + """ + + status_code = 200 + ok = True + + def __init__(self, content, encoding=None): + self.content = content + self.encoding = encoding + + @property + def text(self): + raise AssertionError("an attachment must be carried as bytes, not text") + + +class EncodinglessResponse(AttachmentResponse): + """An attachment response with no ``encoding`` attribute at all. + + What ``getattr(response, "encoding", None)`` at the call site defends + against: a proxy, or anything that never sets the field. + """ + + def __init__(self, content): + self.content = content + + +def health_response(healthy, status_code=200): + """A health-check response as ``_validate_health_endpoint`` reads it.""" + return FakeResponse(status_code=status_code, json_data={"Healthy": bool(healthy)}) + + +def vault_broker_payload(vault_url="https://vault.example.com"): + """The ``/vaultbroker/api/vaults`` body ``ensure_vault_url`` parses.""" + return { + "vaults": [ + {"isDefault": True, "isActive": True, "connection": {"url": vault_url}} + ] + } + + +def vault_broker_response(vault_url="https://vault.example.com"): + """``vault_broker_payload`` as a 200 response.""" + return FakeResponse(json_data=vault_broker_payload(vault_url)) + + +TOKEN_FROM_FAKE_ENDPOINT = "tok-from-fake-token-endpoint" + + +def fake_token_post(url, *args, **kwargs): + """Stand in for ``requests.post`` against an OAuth2 token endpoint. + + Patching only ``requests.get`` would let a grant request reach the real + network with the test's fake credentials, and block for the full timeout. + """ + return FakeResponse( + json_data={"access_token": TOKEN_FROM_FAKE_ENDPOINT, "expires_in": 1200} + ) + + +def make_grant_authorizer( + base_url="https://ss.example.com", username="user", password="pass", **kwargs +): + """A ``PasswordGrantAuthorizer`` with an explicit type, so no probe fires.""" + kwargs.setdefault("server_type", "secret_server") + return PasswordGrantAuthorizer(base_url, username, password, **kwargs) + + +def make_server(base_url, server_type, token="tok"): + """A ``SecretServer`` over a pre-resolved ``AccessTokenAuthorizer``. + + The explicit ``server_type`` means construction issues no health probe, so + the caller's ``requests.get`` patch only ever sees the calls under test. + """ + return SecretServer( + base_url, AccessTokenAuthorizer(token, base_url, server_type=server_type) + ) + + +def join_all(threads, timeout=10): + """Join worker threads with a bound, so a deadlock fails in seconds with + the stuck workers named. + + ``timeout`` is a total budget, not per thread; create threads as daemons. + """ + deadline = time.monotonic() + timeout + for t in threads: + t.join(max(0.0, deadline - time.monotonic())) + stuck = [t.name for t in threads if t.is_alive()] + assert not stuck, f"worker threads did not finish within {timeout}s: {stuck}" diff --git a/tests/test_security_phase1.py b/tests/test_security_phase1.py new file mode 100644 index 0000000..8bbdecb --- /dev/null +++ b/tests/test_security_phase1.py @@ -0,0 +1,728 @@ +"""Offline unit tests for the Phase 1 security-review fixes (see PR #98). + +Covers SDK-1 (timeouts on every call), SDK-3 (refresh before expiry) and SDK-9 +(``.response`` populated). Offline: ``requests`` is patched in the SDK module. +""" + +from datetime import datetime, timedelta, timezone + +import pytest + +from urllib.parse import urlsplit + +from delinea.secrets.server import ( + _MAX_GRANT_LIFETIME_SECONDS, + DEFAULT_REQUEST_TIMEOUT, + AccessTokenAuthorizer, + PasswordGrantAuthorizer, + SecretServer, + SecretServerClientError, + SecretServerError, + SecretServerV0, + _with_query_flag, +) +from fakes import ( + HTTP_GET, + HTTP_POST, + FakeResponse, + fake_token_post, + health_response, + make_grant_authorizer, + make_server, + vault_broker_response, +) + +# Shared fixtures from tests/conftest.py: fail loudly on an unmocked HTTP +# call, and isolate the process-global server-detection cache. +pytestmark = pytest.mark.usefixtures("no_network", "clear_detection_cache") + + +# --------------------------------------------------------------------------- +# SDK-1: timeout coverage +# --------------------------------------------------------------------------- + + +@pytest.fixture +def http_spy(monkeypatch): + """Replace ``requests.get``/``requests.post`` with a recording fake that + serves canned, route-appropriate responses. Returns the list of recorded + (method, url, kwargs) calls.""" + + calls = [] + + def route(url, params=None): + if url.endswith("/api/v1/healthcheck"): + return health_response(False) + if url.endswith("/health"): + return health_response(True) + if url.endswith("/vaultbroker/api/vaults"): + return vault_broker_response() + if url.endswith("/secrets/search-total"): + return FakeResponse(text="3") + if url.endswith("/folders/lookup"): + return FakeResponse( + json_data={"total": 2, "records": [{"id": 7}, {"id": 8}]} + ) + if url.endswith("/secrets"): + return FakeResponse(json_data={"records": [{"id": 1}]}) + if "/secrets/" in url: + return FakeResponse(json_data={"items": []}) + if "/folders/" in url: + return FakeResponse(json_data={"id": 1}) + return FakeResponse(json_data={}) + + def fake_get(url, *args, **kwargs): + calls.append(("GET", url, kwargs)) + return route(url, kwargs.get("params")) + + def fake_post(url, *args, **kwargs): + calls.append(("POST", url, kwargs)) + return fake_token_post(url, *args, **kwargs) + + monkeypatch.setattr(HTTP_GET, fake_get) + monkeypatch.setattr(HTTP_POST, fake_post) + return calls + + +def _server(base_url="https://ss.example.com"): + return make_server(base_url, "secret_server") + + +def test_every_http_call_passes_a_timeout(http_spy): + """Exercise every SecretServer request path and assert an explicit timeout + is passed on each underlying HTTP call (SDK-1).""" + server = _server() + + server.get_secret_json(1) + server.get_secret_json(1, query_params={"a": "b"}) + server.get_folder_json(1, query_params={}) # get_all_children default True + server.get_folder_json(1, query_params={"a": "b"}, get_all_children=False) + server.search_secrets() + server.search_secrets(query_params={"a": "b"}) + server.lookup_folders() + server.lookup_folders(query_params={"a": "b"}) + server.get_secret_ids_by_folderid(2) + server.get_child_folder_ids_by_folderid(2) + + assert len(http_spy) > 0 + # ``timeout=None`` is the exact hang SDK-1 fixed, so "present" is not + # enough: every call must carry the configured value. + missing = [ + (method, url) + for method, url, kwargs in http_spy + if kwargs.get("timeout") != DEFAULT_REQUEST_TIMEOUT + ] + assert missing == [], f"HTTP calls issued without the timeout: {missing}" + + +def test_every_http_call_site_passes_the_timeout(http_spy): + """One lazily detected Platform flow visits all four ``requests`` call + sites: both health probes, the token POST, the vault lookup and an API GET. + The test above pins an explicit ``server_type``, so it reaches only two. + """ + authorizer = PasswordGrantAuthorizer("https://platform.example.com", "u", "p") + server = SecretServer("https://platform.example.com", authorizer) + server.get_secret_json(1) + + paths = {(method, urlsplit(url).path) for method, url, _ in http_spy} + assert paths == { + ("GET", "/api/v1/healthcheck"), + ("GET", "/health"), + ("POST", PasswordGrantAuthorizer.PLATFORM_TOKEN_PATH_URI), + ("GET", "/vaultbroker/api/vaults"), + ("GET", "/api/v1/secrets/1"), + } + assert server.base_url == "https://vault.example.com" + wrong = [ + (method, url, kwargs.get("timeout")) + for method, url, kwargs in http_spy + if kwargs.get("timeout") != DEFAULT_REQUEST_TIMEOUT + ] + assert wrong == [] + + +def test_token_grant_passes_a_timeout(http_spy): + """The OAuth2 token POST must also carry a timeout (SDK-1).""" + grant = make_grant_authorizer() + grant.get_access_token() + + posts = [c for c in http_spy if c[0] == "POST"] + assert len(posts) == 1 + assert posts[0][2].get("timeout") == DEFAULT_REQUEST_TIMEOUT + + +# --------------------------------------------------------------------------- +# SDK-3: refresh drift is subtracted (refresh happens BEFORE expiry) +# --------------------------------------------------------------------------- + + +def _grant_authorizer_with_token(refreshed_seconds_ago, expires_in=1200): + auth = make_grant_authorizer() + auth.access_grant = {"access_token": "old", "expires_in": expires_in} + auth.access_grant_refreshed = datetime.now(timezone.utc) - timedelta( + seconds=refreshed_seconds_ago + ) + # Shadow the grant call on the instance so no network is needed. + auth.get_access_grant = lambda token_url, grant_request: { + "access_token": "new", + "expires_in": expires_in, + } + return auth + + +def test_refresh_fires_inside_drift_window(): + """A token expiring within the 300s drift window is refreshed early.""" + # expires_in=1200, refreshed 901s ago -> 299s of validity left (< 300 drift) + auth = _grant_authorizer_with_token(refreshed_seconds_ago=1200 - 299) + assert auth.get_access_token() == "new" + + +def test_refresh_skipped_outside_drift_window(): + """A token with more than the drift window of validity left is reused.""" + # expires_in=1200, refreshed 899s ago -> 301s of validity left (> 300 drift) + auth = _grant_authorizer_with_token(refreshed_seconds_ago=1200 - 301) + assert auth.get_access_token() == "old" + + +def test_expired_token_is_refreshed(): + """A token past its expiry is never reused (regression guard: the old + ``+ seconds_of_drift`` arithmetic kept expired tokens alive for 300s).""" + auth = _grant_authorizer_with_token(refreshed_seconds_ago=1201) + assert auth.get_access_token() == "new" + + +# --------------------------------------------------------------------------- +# SDK-9: exception plumbing +# --------------------------------------------------------------------------- + + +def test_error_response_attribute_is_set(): + response = FakeResponse(status_code=403) + err = SecretServerError("denied", response) + assert err.response is response + assert err.message == "denied" + + +def test_process_4xx_json_without_message_key(): + """A 4xx JSON body lacking message/error keys must raise a client error + with a fallback message, not ``UnboundLocalError``.""" + response = FakeResponse(status_code=403, json_data={"foo": 1}) + with pytest.raises(SecretServerClientError) as excinfo: + SecretServer.process(response) + assert excinfo.value.response is response + assert "403" in excinfo.value.message + + +def test_process_4xx_json_with_message_key(): + response = FakeResponse(status_code=400, json_data={"message": "bad request"}) + with pytest.raises(SecretServerClientError) as excinfo: + SecretServer.process(response) + assert excinfo.value.message == "bad request" + assert excinfo.value.response is response + + +def test_process_4xx_non_json_body(): + response = FakeResponse(status_code=404, text="not found") + with pytest.raises(SecretServerClientError) as excinfo: + SecretServer.process(response) + assert excinfo.value.response is response + + +# --------------------------------------------------------------------------- +# Review step 1: short-lived grants are not refreshed on every call +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize("expires_in", [300, 60]) +def test_short_lived_grant_is_reused_when_fresh(expires_in): + """``expires_in <= drift`` used to yield a zero/negative validity window, + forcing a token POST on every ``get_access_token()`` call.""" + auth = _grant_authorizer_with_token(refreshed_seconds_ago=0, expires_in=expires_in) + assert auth.get_access_token() == "old" + + +def test_short_lived_grant_refreshes_after_half_lifetime(): + """A short-lived grant is reused for half its lifetime, then refreshed.""" + auth = _grant_authorizer_with_token(refreshed_seconds_ago=31, expires_in=60) + assert auth.get_access_token() == "new" + + +def test_long_lived_grant_still_uses_full_drift(): + validity = PasswordGrantAuthorizer._grant_validity_seconds( + {"expires_in": 1200}, 300 + ) + assert validity == 900 + + +# --------------------------------------------------------------------------- +# Review step 5: expires_in defaults, rejections and warnings +# --------------------------------------------------------------------------- + + +def _token_endpoint(monkeypatch, grant): + response = FakeResponse(status_code=200, json_data=grant) + monkeypatch.setattr(HTTP_POST, lambda *a, **k: response) + return response + + +def test_grant_without_expires_in_defaults_to_one_hour(monkeypatch, caplog): + """RFC 6749 makes ``expires_in`` RECOMMENDED, not required. A grant + without it is assumed to last an hour (and says so at DEBUG) rather + than being re-requested on every API call.""" + _token_endpoint(monkeypatch, {"access_token": "tok"}) + with caplog.at_level("DEBUG", logger="delinea.secrets.server"): + grant = PasswordGrantAuthorizer.get_access_grant( + "https://ss.example.com/oauth2/token", {} + ) + assert grant == {"access_token": "tok", "expires_in": 3600} + assert any("no expires_in" in record.getMessage() for record in caplog.records) + # And the default flows through to the refresh arithmetic. + assert PasswordGrantAuthorizer._grant_validity_seconds(grant, 300) == 3300 + + +def test_grant_with_null_expires_in_is_treated_as_missing(monkeypatch): + _token_endpoint(monkeypatch, {"access_token": "tok", "expires_in": None}) + grant = PasswordGrantAuthorizer.get_access_grant( + "https://ss.example.com/oauth2/token", {} + ) + assert grant["expires_in"] == 3600 + + +def test_directly_assigned_grant_without_expires_in_uses_default(): + """The same default applies to a grant assigned without going through + ``get_access_grant`` (no debug log on this path: it runs per call).""" + auth = _grant_authorizer_with_token(refreshed_seconds_ago=0) + auth.access_grant = {"access_token": "old"} + assert auth.get_access_token() == "old" + auth.access_grant_refreshed -= timedelta(seconds=3301) + assert auth.get_access_token() == "new" + + +@pytest.mark.parametrize( + "bad", + [ + # Not a number at all. + "soon", + "", + True, + False, + {"seconds": 60}, + [3600], + # Numeric but non-finite. + "NaN", + "Infinity", + ], +) +def test_non_numeric_expires_in_is_rejected_at_token_endpoint(monkeypatch, bad): + """A grant whose ``expires_in`` cannot be read as a finite number is + malformed. It is rejected once, here, with the response attached, + instead of being stored and wedging every later call.""" + response = _token_endpoint(monkeypatch, {"access_token": "tok", "expires_in": bad}) + with pytest.raises(SecretServerError) as excinfo: + PasswordGrantAuthorizer.get_access_grant( + "https://ss.example.com/oauth2/token", {} + ) + assert "non-numeric expires_in" in excinfo.value.message + assert excinfo.value.response is response + + +@pytest.mark.parametrize("lifetime", [0, -1, "0", 1e-9]) +def test_non_positive_expires_in_is_honoured_and_warned(monkeypatch, caplog, lifetime): + """``expires_in: 0`` is a token the server issued with no reuse window. + Refusing it would be an outage and assuming an hour would hand the caller + an expired token, so it is honoured and warned once per authorizer. + """ + posts = [] + + def counting_post(url, *a, **k): + posts.append(url) + return FakeResponse( + json_data={"access_token": f"tok-{len(posts)}", "expires_in": lifetime} + ) + + monkeypatch.setattr(HTTP_POST, counting_post) + auth = make_grant_authorizer() + with caplog.at_level("WARNING", logger="delinea.secrets.server"): + tokens = [auth.get_access_token() for _ in range(3)] + assert tokens == ["tok-1", "tok-2", "tok-3"] # every call works... + assert len(posts) == 3 # ...at the cost the server asked for + warnings_ = [r for r in caplog.records if "re-requested on every" in r.getMessage()] + assert len(warnings_) == 1 and warnings_[0].levelname == "WARNING" + # A second authorizer against the same server warns on its own. + other = PasswordGrantAuthorizer( + "https://ss.example.com", "user2", "pass", server_type="secret_server" + ) + with caplog.at_level("WARNING", logger="delinea.secrets.server"): + other.get_access_token() + assert ( + len([r for r in caplog.records if "re-requested on every" in r.getMessage()]) + == 2 + ) + + +def test_numeric_string_expires_in_is_accepted(monkeypatch): + """Some OAuth2 servers serialize the field as a string.""" + _token_endpoint(monkeypatch, {"access_token": "tok", "expires_in": "1200"}) + grant = PasswordGrantAuthorizer.get_access_grant( + "https://ss.example.com/oauth2/token", {} + ) + assert grant["expires_in"] == "1200" + assert PasswordGrantAuthorizer._grant_validity_seconds(grant, 300) == 900 + + +def test_non_numeric_expires_in_error_detail_is_capped(monkeypatch): + _token_endpoint(monkeypatch, {"access_token": "tok", "expires_in": "x" * 5000}) + with pytest.raises(SecretServerError) as excinfo: + PasswordGrantAuthorizer.get_access_grant( + "https://ss.example.com/oauth2/token", {} + ) + assert len(excinfo.value.message) < 400 + + +# --------------------------------------------------------------------------- +# Review step 2: SecretServerError contract is uniform on every raise path +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize("body", ["null", "5", '"Access denied"', '["error"]', "true"]) +def test_process_4xx_non_object_json_raises_client_error(body): + """A 4xx body that is valid JSON but not an object must not escape as + ``TypeError``; it is a client error with the status fallback message.""" + response = FakeResponse(status_code=403, text=body) + with pytest.raises(SecretServerClientError) as excinfo: + SecretServer.process(response) + assert excinfo.value.response is response + assert excinfo.value.message == "HTTP 403" + + +def test_process_4xx_non_string_message_key_falls_back(): + response = FakeResponse(status_code=400, json_data={"message": {"code": 1}}) + with pytest.raises(SecretServerClientError) as excinfo: + SecretServer.process(response) + assert excinfo.value.message == "HTTP 400" + + +def test_process_5xx_populates_response_and_message(): + from delinea.secrets.server import SecretServerServiceError + + response = FakeResponse(status_code=500, text="" + "x" * 500) + with pytest.raises(SecretServerServiceError) as excinfo: + SecretServer.process(response) + err = excinfo.value + assert err.response is response + assert err.message.startswith("HTTP 500: ") + assert err.message.endswith("...[truncated]") + assert len(err.message) < 300 + assert str(err) == err.message + assert "login" in err.message + assert "" in excinfo.value.message) is ( + not redacted and body.text.startswith("") + ) + + +@pytest.mark.parametrize( + "lifetime, warned", [(2, False), (1, False), (0.5, True), (0, True), (-1, True)] +) +def test_short_lifetime_warning_only_without_a_reuse_window( + monkeypatch, caplog, lifetime, warned +): + """The warning describes a token re-requested on every call, so it must + fire on the lifetime the server sent, not on the drift-adjusted window.""" + monkeypatch.setattr( + HTTP_POST, + lambda *a, **k: FakeResponse( + json_data={"access_token": "tok", "expires_in": lifetime} + ), + ) + auth = make_grant_authorizer() + with caplog.at_level("WARNING", logger="delinea.secrets.server"): + auth.get_access_token() + fired = any("re-requested on every" in r.getMessage() for r in caplog.records) + assert fired is warned + + +def test_folder_count_errors_carry_the_response(monkeypatch): + """Every error on the folder paths attaches the response it describes.""" + bodies = { + "total": FakeResponse(text="abc"), + "lookup": FakeResponse(json_data={"total": True}), + } + + def fake_get(url, *a, **k): + return ( + bodies["total"] + if url.endswith("/secrets/search-total") + else bodies["lookup"] + ) + + monkeypatch.setattr(HTTP_GET, fake_get) + server = make_server("https://ss.example.com", "secret_server") + with pytest.raises(SecretServerError) as count_error: + server.get_secret_ids_by_folderid(2) + assert count_error.value.response is bodies["total"] + with pytest.raises(SecretServerError) as total_error: + server.get_child_folder_ids_by_folderid(2) + assert total_error.value.response is bodies["lookup"] + + +def test_non_datetime_refresh_timestamp_reads_as_stale(): + auth = make_grant_authorizer() + auth.access_grant = {"access_token": "old", "expires_in": 1200} + auth.access_grant_refreshed = "yesterday" + auth.get_access_grant = lambda *a, **k: {"access_token": "new", "expires_in": 1200} + assert auth.get_access_token() == "new" diff --git a/tests/test_security_phase2.py b/tests/test_security_phase2.py new file mode 100644 index 0000000..f7866e3 --- /dev/null +++ b/tests/test_security_phase2.py @@ -0,0 +1,587 @@ +"""Offline unit tests for the Phase 2 security-review fixes (see PR #98). + +Covers SDK-2 (a warning on plaintext http), SDK-4 (health checks need a 2xx and +an exact match), SDK-6 (bodies capped in messages), SDK-7 (https vault URLs). +""" + +import pytest + +from delinea.secrets.server import ( + AccessTokenAuthorizer, + PasswordGrantAuthorizer, + SecretServer, + SecretServerError, +) +from fakes import ( + HTTP_GET, + JSON_NULL, + BytesOnlyResponse, + FakeResponse, + HostileBody, + make_server, + vault_broker_payload, + vault_broker_response, +) + +# Shared fixtures from tests/conftest.py: fail loudly on an unmocked HTTP +# call, and isolate the process-global server-detection cache. +pytestmark = pytest.mark.usefixtures("no_network", "clear_detection_cache") + + +# --------------------------------------------------------------------------- +# SDK-2: warn on non-https base_url +# --------------------------------------------------------------------------- + + +def test_access_token_authorizer_warns_on_http(): + with pytest.warns(UserWarning, match="does not use https"): + AccessTokenAuthorizer("tok", "http://ss.example.com", server_type="platform") + + +def test_access_token_authorizer_no_warning_on_https(recwarn): + AccessTokenAuthorizer("tok", "https://ss.example.com", server_type="platform") + assert len(recwarn) == 0 + + +def test_password_grant_authorizer_warns_on_http(): + with pytest.warns(UserWarning, match="does not use https"): + PasswordGrantAuthorizer( + "http://ss.example.com", "user", "pass", server_type="platform" + ) + + +def test_secret_server_warns_on_http(): + authorizer = AccessTokenAuthorizer( + "tok", "https://ss.example.com", server_type="platform" + ) + with pytest.warns(UserWarning, match="does not use https"): + SecretServer("http://ss.example.com", authorizer) + + +def test_secret_server_no_warning_on_https(recwarn): + authorizer = AccessTokenAuthorizer( + "tok", "https://ss.example.com", server_type="platform" + ) + recwarn.clear() + SecretServer("https://ss.example.com", authorizer) + assert len(recwarn) == 0 + + +# --------------------------------------------------------------------------- +# SDK-4: health-check validation tightened +# --------------------------------------------------------------------------- + + +def _probe(monkeypatch, response): + """Drive ``_validate_health_endpoint`` on a real authorizer instance + (constructed via an explicit server_type override so no probe fires + during construction itself).""" + monkeypatch.setattr(HTTP_GET, lambda *a, **k: response) + authorizer = AccessTokenAuthorizer( + "tok", "https://x.example.com", server_type="platform" + ) + return authorizer._validate_health_endpoint("https://x.example.com/health") + + +def test_health_check_rejects_unhealthy_substring(monkeypatch): + """A body containing "Unhealthy" must NOT be treated as healthy (the old + substring check ``b"healthy" in body`` incorrectly matched it).""" + response = FakeResponse(status_code=200, text="Unhealthy") + assert _probe(monkeypatch, response) is False + + +def test_health_check_rejects_non_2xx_even_with_healthy_body(monkeypatch): + response = FakeResponse(status_code=500, text="Healthy") + assert _probe(monkeypatch, response) is False + + +def test_health_check_rejects_json_healthy_false(monkeypatch): + response = FakeResponse(status_code=200, json_data={"Healthy": False}) + assert _probe(monkeypatch, response) is False + + +def test_health_check_accepts_plain_healthy_text(monkeypatch): + response = FakeResponse(status_code=200, text="Healthy") + assert _probe(monkeypatch, response) is True + + +def test_health_check_accepts_json_healthy_true(monkeypatch): + response = FakeResponse(status_code=200, json_data={"Healthy": True}) + assert _probe(monkeypatch, response) is True + + +def test_health_check_probe_exception_is_unhealthy(monkeypatch): + def raise_get(*a, **k): + raise ConnectionError("boom") + + # server_type="platform" skips probing during construction; only the + # explicit _validate_health_endpoint call below is under test. + authorizer = AccessTokenAuthorizer( + "tok", "https://x.example.com", server_type="platform" + ) + monkeypatch.setattr(HTTP_GET, raise_get) + assert authorizer._validate_health_endpoint("https://x.example.com/health") is False + + +# --------------------------------------------------------------------------- +# SDK-6: response bodies sanitized out of exception messages +# --------------------------------------------------------------------------- + + +def _platform_server(monkeypatch, vault_url="https://vault.example.com"): + """Build a SecretServer wired to a platform authorizer, with + requests.get mocked to serve a vault-broker response.""" + server = make_server("https://platform.example.com", "platform") + + def fake_get(url, *args, **kwargs): + if "vaultbroker" in url: + return vault_broker_response(vault_url) + return FakeResponse(json_data={}) + + monkeypatch.setattr(HTTP_GET, fake_get) + return server + + +def test_vault_fetch_failure_truncates_body(monkeypatch): + server = make_server("https://platform.example.com", "platform") + huge_body = "x" * 5000 + + monkeypatch.setattr( + HTTP_GET, + lambda *a, **k: FakeResponse(status_code=500, text=huge_body), + ) + + with pytest.raises(SecretServerError) as excinfo: + server.ensure_vault_url() + assert "...[truncated]" in str(excinfo.value) + assert len(str(excinfo.value)) < len(huge_body) + + +def test_get_secret_json_decode_failure_has_no_body(monkeypatch): + server = make_server("https://ss.example.com", "secret_server") + secret_marker = "TOP-SECRET-VALUE" + + monkeypatch.setattr( + HTTP_GET, + lambda *a, **k: FakeResponse(status_code=200, text=secret_marker), + ) + + with pytest.raises(SecretServerError) as excinfo: + server.get_secret(1, fetch_file_attachments=False) + assert secret_marker not in str(excinfo.value) + + +def test_get_folder_json_decode_failure_is_truncated_not_omitted(monkeypatch): + server = make_server("https://ss.example.com", "secret_server") + + monkeypatch.setattr( + HTTP_GET, + lambda *a, **k: FakeResponse(status_code=200, text="not json"), + ) + + with pytest.raises(SecretServerError) as excinfo: + server.get_folder(1, query_params={}) + assert "not json" in str(excinfo.value) + + +# --------------------------------------------------------------------------- +# SDK-7: vault-broker redirect URL must be a valid https URL +# --------------------------------------------------------------------------- + + +def test_vault_url_rejects_http(monkeypatch): + server = _platform_server(monkeypatch, vault_url="http://evil.example.com") + with pytest.raises(SecretServerError, match="https"): + server.ensure_vault_url() + + +def test_vault_url_accepts_https(monkeypatch): + server = _platform_server(monkeypatch, vault_url="https://vault.example.com") + server.ensure_vault_url() + assert server.base_url == "https://vault.example.com" + + +# --------------------------------------------------------------------------- +# Health-check body forms: exactly the two shapes the products emit +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + "json_data", + [ + # Looser matches, briefly accepted during review and then reverted: + # neither product emits them, and a string ``"true"`` or a bare + # ``"Healthy"`` is what an error page or captive portal may produce. + "Healthy", + {"Healthy": "true"}, + {"Healthy": "false"}, + {"Healthy": 1}, + {"Healthy": None}, + # Other JSON shapes that are not the Secret Server object. + ["Healthy"], + 1, + 0, + {"healthy": True}, + ], +) +def test_health_check_rejects_other_json_shapes(monkeypatch, json_data): + response = FakeResponse(status_code=200, json_data=json_data) + assert _probe(monkeypatch, response) is False + + +def test_health_check_rejects_healthy_with_extra_text(monkeypatch): + response = FakeResponse(status_code=200, text="Status: Healthy") + assert _probe(monkeypatch, response) is False + + +# --------------------------------------------------------------------------- +# Review step 5: the insecure-URL warning is attributed to the caller +# --------------------------------------------------------------------------- + + +def _warning_basenames(record): + import os + + return {os.path.basename(w.filename) for w in record} + + +def _this_file(): + import os + + return os.path.basename(__file__) + + +def test_direct_construction_warning_points_at_caller(): + with pytest.warns(UserWarning, match="does not use https") as record: + AccessTokenAuthorizer( + "tok", "http://direct.example.com", server_type="platform" + ) + assert _warning_basenames(record) == {_this_file()} + + +def test_legacy_wrapper_warning_points_at_caller(): + """``SecretServerV0`` adds a frame between the caller and the warning; a + fixed ``stacklevel`` reported a line inside ``server.py`` instead.""" + from delinea.secrets.server import SecretServerV0 + + with pytest.warns(UserWarning, match="does not use https") as record: + SecretServerV0("http://legacy.example.com", "user", "pass") + + assert _warning_basenames(record) == {_this_file()} + assert "server.py" not in _warning_basenames(record) + + +def test_cloud_wrapper_warning_points_at_caller(): + from delinea.secrets.server import SecretServerCloud + + authorizer = AccessTokenAuthorizer( + "tok", "https://platform.example.com", server_type="platform" + ) + with pytest.warns(UserWarning, match="does not use https") as record: + SecretServerCloud(authorizer=authorizer, base_url="http://cloud.example.com") + + assert _warning_basenames(record) == {_this_file()} + + +def test_domain_authorizer_warning_points_at_caller(): + from delinea.secrets.server import DomainPasswordGrantAuthorizer + + with pytest.warns(UserWarning, match="does not use https") as record: + DomainPasswordGrantAuthorizer( + "http://domain.example.com", + "user", + "example.com", + "pass", + server_type="secret_server", + ) + + assert _warning_basenames(record) == {_this_file()} + + +def test_https_wrapper_emits_no_warning(recwarn): + from delinea.secrets.server import SecretServerV0 + + recwarn.clear() + SecretServerV0("https://legacy.example.com", "user", "pass") + assert len(recwarn) == 0 + + +# --------------------------------------------------------------------------- +# Review step 4: the vault-fetch error path, and capped body excerpts +# --------------------------------------------------------------------------- + + +def test_vault_fetch_failure_populates_response(monkeypatch): + server = make_server("https://platform.example.com", "platform") + response = FakeResponse(status_code=500, text="x" * 5000) + + monkeypatch.setattr(HTTP_GET, lambda *a, **k: response) + + with pytest.raises(SecretServerError) as excinfo: + server.ensure_vault_url() + err = excinfo.value + assert err.response is response + assert "...[truncated]" in err.message + assert len(err.message) < 400 + + +def test_vault_fetch_failure_excerpts_bytes_without_decoding(monkeypatch): + server = make_server("https://platform.example.com", "platform") + response = BytesOnlyResponse(b"" + b"x" * 5_000_000) + + monkeypatch.setattr(HTTP_GET, lambda *a, **k: response) + + with pytest.raises(SecretServerError) as excinfo: + server.ensure_vault_url() + err = excinfo.value + assert err.message.startswith("Failed to fetch vault details: HTTP 502: ") + assert err.message.endswith("...[truncated]") + assert len(err.message) < 400 + + +def test_body_excerpt_marks_truncation_for_multibyte_bodies(): + """Slicing bytes before decoding must still leave enough characters to + show the body ran over the limit. A ``limit + 1`` byte slice did not: a + 12 000-character UTF-8 page came back as 134 unmarked characters. + """ + from delinea.secrets.server import _safe_body_excerpt_bytes + + excerpt = _safe_body_excerpt_bytes(("caf\u00e9 " * 3000).encode("utf-8")) + assert excerpt.endswith("...[truncated]") + assert excerpt.startswith("caf\u00e9") + assert len(excerpt) < 300 + + +def test_body_excerpt_leaves_short_multibyte_body_unmarked(): + from delinea.secrets.server import _safe_body_excerpt_bytes + + assert _safe_body_excerpt_bytes("caf\u00e9".encode("utf-8")) == "caf\u00e9" + + +def test_describe_response_reads_bytes_not_text(): + """``_describe_response`` runs on the 5xx and token-grant paths, so it must + not decode and charset-sniff a whole multi-megabyte error page.""" + from delinea.secrets.server import _describe_response + + message = _describe_response(BytesOnlyResponse(b"" + b"x" * 5_000_000)) + assert message.startswith("HTTP 502: ") + assert message.endswith("...[truncated]") + assert len(message) < 400 + + +def test_health_check_unreadable_body_is_unhealthy(monkeypatch): + """The guard around body inspection returns False rather than letting an + unexpected error abort detection.""" + assert _probe(monkeypatch, HostileBody()) is False + + +def test_describe_response_keeps_a_latin1_tail_that_looks_utf8(): + """A Latin-1 body ending in a UTF-8 lead byte must not lose its tail. + + A non-final incremental decode buffers that byte and reports success, so + the excerpt silently came back short with no truncation marker. + """ + from delinea.secrets.server import _safe_body_excerpt_bytes + + body = "Erreur: acc\u00e8s refus\u00e9".encode("iso-8859-1") + + assert _safe_body_excerpt_bytes(body, encoding="ISO-8859-1") == ( + "Erreur: acc\u00e8s refus\u00e9" + ) + assert _safe_body_excerpt_bytes(b"\xc3", encoding="ISO-8859-1") == "\u00c3" + + +@pytest.mark.parametrize( + "declared", + ["ISO-8859-1", "latin-1", "latin", "iso8859", "csisolatin1", "L1", "cp819"], +) +def test_describe_response_reads_utf8_declared_as_requests_latin1_default( + declared, +): + """``requests`` reports ISO-8859-1 for any ``text/*`` body with no charset. + A UTF-8 error page from a proxy or IIS must not come back as mojibake + because of that default, whichever Latin-1 alias was declared. + """ + from delinea.secrets.server import _describe_response + + class Utf8ButDeclaredLatin1: + status_code = 502 + ok = False + encoding = declared # what requests fills in, not the server + content = "Fehler: Zugriff verweigert f\u00fcr n\u00e9".encode("utf-8") + + def json(self): + raise ValueError("no JSON body") + + assert ( + _describe_response(Utf8ButDeclaredLatin1()) + == "HTTP 502: Fehler: Zugriff verweigert f\u00fcr n\u00e9" + ) + + +@pytest.mark.parametrize("wide", ["utf-32-le", "utf-32", "utf-16"]) +def test_body_excerpt_keeps_truncation_marker_for_wide_encodings(wide): + """A body that was cut must say so even when the cut bytes decode to + ``limit`` characters or fewer: with a BOM (``utf-32``) the preamble + eats four of the sliced bytes, so counting characters is not enough.""" + from delinea.secrets.server import _safe_body_excerpt_bytes + + body = ("x" * 12000).encode(wide) + excerpt = _safe_body_excerpt_bytes(body, limit=200, encoding=wide) + assert excerpt.endswith("...[truncated]") + assert excerpt.startswith("x" * 200) + assert excerpt.count("...[truncated]") == 1 + + +def test_body_excerpt_has_no_marker_when_nothing_was_cut(): + from delinea.secrets.server import _safe_body_excerpt_bytes + + assert _safe_body_excerpt_bytes(b"short", limit=200) == "short" + exact = ("y" * 200).encode("utf-32") # 804 bytes: fits the slice exactly + assert _safe_body_excerpt_bytes(exact, limit=200, encoding="utf-32") == "y" * 200 + + +def test_describe_response_honours_declared_encoding(): + """A proxy's Latin-1 error page must read correctly, not as U+FFFD.""" + from delinea.secrets.server import _describe_response + + class Latin1Response: + status_code = 500 + ok = False + encoding = "iso-8859-1" + content = "Erreur: acc\u00e8s refus\u00e9".encode("iso-8859-1") + + def json(self): + raise ValueError("no JSON body") + + assert ( + _describe_response(Latin1Response()) + == "HTTP 500: Erreur: acc\u00e8s refus\u00e9" + ) + + +@pytest.mark.parametrize( + "charset", + [ + "not-a-real-charset", # unknown codec -> LookupError + "idna", # registered codec that rejects errors="replace" -> UnicodeError + "punycode", # registered codec that rejects non-ASCII -> UnicodeDecodeError + "", # empty charset parameter + 5, # not even a string + "ut\x00f8", # a NUL byte survives header parsing -> ValueError + "\ud800", # a lone surrogate -> UnicodeEncodeError from codecs.lookup + ], +) +def test_describe_response_falls_back_to_utf8_for_unusable_encoding(charset): + """``response.encoding`` is copied verbatim from the server's + ``charset=`` parameter, so any codec name (or none) can arrive. None + of them may escape ``_describe_response`` as a codec error.""" + from delinea.secrets.server import _describe_response + + class OddEncoding: + status_code = 500 + ok = False + encoding = charset + content = b"Bad \xe9 gateway" # one non-UTF-8 byte + + def json(self): + raise ValueError("no JSON body") + + assert _describe_response(OddEncoding()) == "HTTP 500: Bad \ufffd gateway" + + +def test_process_error_with_hostile_charset_is_a_secret_server_error(): + """The whole path a proxy or WAF error page would take: a 5xx whose + Content-Type names a non-text codec must still surface as the error + callers are told to catch.""" + + class IdnaError: + status_code = 502 + ok = False + encoding = "idna" + content = b"\xffBad Gateway" + text = "Bad Gateway" + + def json(self): + raise ValueError("no JSON body") + + with pytest.raises(SecretServerError) as excinfo: + SecretServer.process(IdnaError()) + assert "Bad Gateway" in excinfo.value.message + + +def test_health_check_rejects_3xx_even_though_requests_calls_it_ok(monkeypatch): + """``requests.Response.ok`` is true below 400; detection requires 2xx.""" + response = FakeResponse(status_code=304, text="Healthy") + assert response.ok + assert _probe(monkeypatch, response) is False + + +def _vault_with_url(url): + return vault_broker_payload(url) + + +def test_vault_switch_logs_the_accepted_host(monkeypatch, caplog): + """Every later API call carries the bearer token to this host, so the + log line that announces the switch must say which host it is.""" + server = make_server("https://platform.example.com", "platform") + monkeypatch.setattr( + HTTP_GET, + lambda *a, **k: vault_broker_response("https://user:pw@vault.example.com"), + ) + with caplog.at_level("INFO", logger="delinea.secrets.server"): + server.ensure_vault_url() + switch = [ + r.getMessage() for r in caplog.records if "Switching base_url" in r.getMessage() + ] + assert switch == [ + "Switching base_url to platform vault connection URL at vault.example.com" + ] + assert "user:pw" not in caplog.text # userinfo never reaches the log + + +def test_non_string_vault_url_is_reported_as_invalid(monkeypatch): + server = make_server("https://platform.example.com", "platform") + response = FakeResponse(json_data=_vault_with_url({"host": "evil.example.net"})) + monkeypatch.setattr(HTTP_GET, lambda *a, **k: response) + with pytest.raises(SecretServerError) as excinfo: + server.ensure_vault_url() + assert "not a valid https URL" in excinfo.value.message + assert excinfo.value.response is response + assert server.base_url == "https://platform.example.com" # unchanged + + +@pytest.mark.parametrize( + "payload", + [ + {"vaults": [{"isDefault": True, "isActive": True, "connection": None}]}, + {"vaults": None}, + {"vaults": [None]}, + [], + None, # no JSON body at all: json() raises + JSON_NULL, # the JSON literal ``null``: json() returns None + # ``connection.url`` present but not a string: must not reach + # ``urlsplit`` and escape as a TypeError/AttributeError. + _vault_with_url({"host": "evil.example.net"}), + _vault_with_url(["https://evil.example.net"]), + _vault_with_url(42), + _vault_with_url(True), + # A netloc with no host: ``urlsplit`` accepts it, ``requests`` would + # raise InvalidURL on the first API call after the switch. + _vault_with_url("https://@"), + _vault_with_url("https://user:pw@"), + # ``urlsplit`` itself raises ValueError for these. + _vault_with_url("https://[oops"), + _vault_with_url("https://a\u2100b/"), + ], +) +def test_vault_payload_shape_errors_are_secret_server_errors(monkeypatch, payload): + """A malformed vault-broker body raises the error callers are told to + catch, never an AttributeError from inside the SDK.""" + server = make_server("https://platform.example.com", "platform") + monkeypatch.setattr( + HTTP_GET, + lambda *a, **k: FakeResponse(json_data=payload), + ) + with pytest.raises(SecretServerError): + server.ensure_vault_url() diff --git a/tests/test_security_phase4.py b/tests/test_security_phase4.py new file mode 100644 index 0000000..2130ce6 --- /dev/null +++ b/tests/test_security_phase4.py @@ -0,0 +1,1489 @@ +"""Offline unit tests for the Phase 4 housekeeping fixes (see PR #98). + +Covers thread-safe refresh, timezone-aware expiry, mutable default arguments, +``get_folder_json`` with no params, attachment bytes and non-numeric totals. +""" + +import copy +import json +import pickle +import threading +import warnings +from datetime import datetime, timedelta, timezone + +import pytest +import requests + +from delinea.secrets.server import ( + AccessTokenAuthorizer, + Authorizer, + FileAttachment, + PasswordGrantAuthorizer, + SecretServer, + SecretServerClientError, + SecretServerError, + SecretServerV0, +) +from fakes import ( + HTTP_GET, + HTTP_POST, + AttachmentResponse, + EncodinglessResponse, + FakeResponse, + fake_token_post, + health_response, + join_all, + make_grant_authorizer, + make_server, + vault_broker_response, +) + +# Shared fixtures from tests/conftest.py: fail loudly on an unmocked HTTP +# call, and isolate the process-global server-detection cache. +pytestmark = pytest.mark.usefixtures("no_network", "clear_detection_cache") + + +# --------------------------------------------------------------------------- +# 4.1 / 4.2: thread-safe, UTC-aware token refresh +# --------------------------------------------------------------------------- + + +def test_refresh_is_thread_safe_and_grants_once(monkeypatch): + """20 threads on a fresh authorizer must grant exactly once: the first in + holds ``_refresh_lock`` while it fetches, the rest then find a valid grant. + The fetch is held open so they pile up; an instant fake hid a missing lock. + """ + import time + + grant_calls = {"count": 0} + + def fake_get_access_grant(token_url, grant_request): + grant_calls["count"] += 1 + time.sleep(0.05) + return {"access_token": f"tok-{grant_calls['count']}", "expires_in": 1200} + + auth = make_grant_authorizer() + monkeypatch.setattr(auth, "get_access_grant", fake_get_access_grant) + + results = [] + errors = [] + start = threading.Event() + + def worker(): + start.wait() + try: + results.append(auth.get_access_token()) + except Exception as exc: # pragma: no cover - failure path + errors.append(exc) + + threads = [threading.Thread(target=worker, daemon=True) for _ in range(20)] + for t in threads: + t.start() + start.set() + join_all(threads) + + assert errors == [] + assert len(results) == 20 + # No thread must observe a torn/partial access_grant. + assert all(r == results[0] for r in results) + assert grant_calls["count"] == 1 + + +def test_access_grant_refreshed_is_timezone_aware(monkeypatch): + monkeypatch.setattr( + PasswordGrantAuthorizer, + "get_access_grant", + staticmethod( + lambda token_url, grant_request: { + "access_token": "tok", + "expires_in": 1200, + } + ), + ) + auth = make_grant_authorizer() + auth.get_access_token() + + assert auth.access_grant_refreshed.tzinfo is not None + # Comparable against an aware "now" without raising TypeError. + assert auth.access_grant_refreshed <= datetime.now(timezone.utc) + + +# --------------------------------------------------------------------------- +# 4.3: mutable default arguments don't leak state +# --------------------------------------------------------------------------- + + +def test_headers_default_not_shared_between_calls(): + auth = AccessTokenAuthorizer( + "tok", "https://ss.example.com", server_type="secret_server" + ) + first = auth.headers() + first["Poisoned"] = "yes" + + second = auth.headers() + assert "Poisoned" not in second + + +# --------------------------------------------------------------------------- +# 4.4: get_folder_json tolerates the None/True default combination +# --------------------------------------------------------------------------- + + +def test_get_folder_json_bare_call_does_not_raise(monkeypatch): + calls = [] + + def fake_get(url, *args, **kwargs): + calls.append((url, kwargs.get("params"))) + return FakeResponse(json_data={"id": 1}) + + monkeypatch.setattr(HTTP_GET, fake_get) + + server = make_server("https://ss.example.com", "secret_server") + + # No query_params, default get_all_children=True: must not raise TypeError. + result = server.get_folder_json(1) + assert result == '{"id": 1}' + url, params = calls[-1] + assert url.endswith("/folders/1") + assert params == {"getAllChildren": "true"} + + +# --------------------------------------------------------------------------- +# 4.5: file-attachment itemValue is the file, not a Response object +# --------------------------------------------------------------------------- + + +# Passed as a field's encoding to get a response object that has none. +_NO_ENCODING = object() + +# The one-file secret most of these tests want. +_ONE_FILE = ("file-slug", b"file-bytes", None, None) + + +class _InitAttachment(FileAttachment): + """A subclass whose ``__init__`` alone takes an argument of its own. + + Rebuilding through ``__new__`` must not re-run it. At module level so + ``pickle`` can find it by name. + """ + + def __new__(cls, data, extra=None, **kwargs): + return super().__new__(cls, data, **kwargs) + + def __init__(self, data, extra, **kwargs): + self.extra = extra + + +class _TaggedAttachment(FileAttachment): + """An attachment subclass with an attribute of its own. + + At module level so ``pickle`` can find it by name. + """ + + def __new__(cls, data, tag=None, **kwargs): + attachment = super().__new__(cls, data, **kwargs) + attachment.tag = tag + return attachment + + +def _attachment_server(monkeypatch, files, seen=None, statuses=None): + """Build a server whose secret has the given file fields and a password. + + ``files`` holds ``(slug, content, filename, encoding)`` per file; ``seen`` + records ``(slug, params)`` per request, ``"secret"`` for the body itself. + """ + fields = [] + for index, (slug, content, filename, encoding) in enumerate(files, start=11): + field = {"fileAttachmentId": index, "slug": slug, "itemValue": None} + if filename is not None: + field["filename"] = filename + fields.append((field, content, encoding)) + password = {"fileAttachmentId": 0, "slug": "password", "itemValue": "p@ss"} + + def fake_get(url, *args, **kwargs): + for field, content, encoding in fields: + if not url.endswith(f"/fields/{field['slug']}"): + continue + if seen is not None: + seen.append((field["slug"], kwargs.get("params"))) + status = (statuses or {}).get(field["slug"], 200) + if status != 200: + return FakeResponse(status_code=status, json_data={"message": "no"}) + if encoding is _NO_ENCODING: + return EncodinglessResponse(content) + return AttachmentResponse(content, encoding=encoding) + if seen is not None: + seen.append(("secret", kwargs.get("params"))) + items = [field for field, _, _ in fields] + [password] + return FakeResponse(json_data={"id": 7, "items": items}) + + monkeypatch.setattr(HTTP_GET, fake_get) + return make_server("https://ss.example.com", "secret_server") + + +def _fetch_attachment(monkeypatch, content, encoding=None, filename=None): + files = [("file-slug", content, filename, encoding)] + server = _attachment_server(monkeypatch, files) + secret = server.get_secret(1, fetch_file_attachments=True) + return secret["items"][0]["itemValue"] + + +def test_file_attachment_item_value_is_the_file_bytes(monkeypatch): + """Never the ``Response``: its repr used to be what got stored.""" + item_value = _fetch_attachment(monkeypatch, b"file-bytes") + + assert isinstance(item_value, bytes) + assert isinstance(item_value, FileAttachment) + assert item_value == b"file-bytes" + + +def test_file_attachment_exposes_content_and_text(monkeypatch): + """The two accessors consumers of the old ``Response`` already call.""" + item_value = _fetch_attachment(monkeypatch, b"file-bytes") + + assert item_value.content == b"file-bytes" + assert type(item_value.content) is bytes + assert item_value.text == "file-bytes" + + +def test_binary_file_attachment_is_written_back_byte_for_byte(monkeypatch, tmp_path): + """The Ansible community.general tss flow: read ``.content``, write "wb". + + These bytes are not valid UTF-8, so the previous ``.text`` lost them. + """ + png = b"\x89PNG\r\n\x1a\n\xff\xfe\x00binary" + + item_value = _fetch_attachment(monkeypatch, png) + + destination = tmp_path / "1_file-slug" + with open(destination, "wb") as handle: + handle.write(item_value.content) + assert destination.read_bytes() == png + + +def test_file_attachment_text_falls_back_to_the_declared_latin_1(monkeypatch): + """Not valid UTF-8, so the strict attempt fails and Latin-1 is used.""" + item_value = _fetch_attachment( + monkeypatch, "caf\u00e9".encode("iso-8859-1"), encoding="iso-8859-1" + ) + + assert item_value.text == "caf\u00e9" + + +def test_file_attachment_text_decodes_a_declared_multibyte_charset(monkeypatch): + """A charset the server really declared must be honoured as given.""" + item_value = _fetch_attachment( + monkeypatch, "caf\u00e9".encode("utf-16"), encoding="utf-16" + ) + + assert item_value.text == "caf\u00e9" + + +def test_attachment_response_without_an_encoding_attribute(monkeypatch): + """The call site reads it with ``getattr``, so a missing one is ``None``.""" + assert not hasattr(EncodinglessResponse(b""), "encoding") + + item_value = _fetch_attachment(monkeypatch, b"file-bytes", encoding=_NO_ENCODING) + + assert item_value.encoding is None + assert item_value.text == "file-bytes" + + +@pytest.mark.parametrize("declared", ["ISO-8859-1", "latin1", "cp819", "8859"]) +def test_file_attachment_text_prefers_utf8_over_a_declared_latin_1( + monkeypatch, declared +): + """``requests`` labels every charset-less ``text/*`` body Latin-1. + + Taking that at face value turns a UTF-8 text attachment into mojibake, + whichever of the codec's many aliases the server happened to send. + """ + item_value = _fetch_attachment( + monkeypatch, "caf\u00e9".encode("utf-8"), encoding=declared + ) + + assert item_value.text == "caf\u00e9" + + +def test_file_attachment_text_replaces_what_the_declared_charset_rejects(monkeypatch): + """``errors="replace"`` on the declared codec, not a silent UTF-8 retry.""" + item_value = _fetch_attachment( + monkeypatch, "caf\u00e9".encode("utf-8"), encoding="ascii" + ) + + assert item_value.text == "caf\ufffd\ufffd" + + +def test_attachment_with_an_unreadable_body_is_empty_not_a_crash(monkeypatch): + """``Response.content`` is ``None`` when ``raw`` is, which ``process`` + does not screen; the ``.text`` this replaced returned ``""`` there. + """ + item_value = _fetch_attachment(monkeypatch, None) + + assert item_value == b"" + assert item_value.text == "" + + +def test_file_attachment_text_survives_a_non_string_declared_charset(monkeypatch): + """``bytes.decode`` raises ``TypeError``, not ``LookupError``, on these.""" + item_value = _fetch_attachment(monkeypatch, b"file-bytes", encoding=b"utf-8") + + assert item_value.text == "file-bytes" + + +def test_zero_byte_attachment_is_an_empty_attachment(monkeypatch): + """Empty, not missing: falsy as bytes, so callers must test the id.""" + item_value = _fetch_attachment(monkeypatch, b"", filename="empty.txt") + + assert isinstance(item_value, FileAttachment) + assert item_value.content == b"" + assert item_value.text == "" + assert not item_value + assert repr(item_value) == "" + + +def test_file_attachment_text_survives_an_unusable_declared_charset(monkeypatch): + """An unknown codec falls back to UTF-8 rather than raising at access.""" + item_value = _fetch_attachment( + monkeypatch, "caf\u00e9".encode("utf-8"), encoding="not-a-real-codec" + ) + + assert item_value.text == "caf\u00e9" + + +def test_file_attachment_text_replaces_undecodable_bytes(monkeypatch): + """``.text`` must not raise on a binary attachment; ``.content`` is exact.""" + item_value = _fetch_attachment(monkeypatch, b"\xff\xfe\x00") + + assert "\ufffd" in item_value.text + assert item_value.content == b"\xff\xfe\x00" + + +def test_file_attachment_repr_withholds_the_contents(monkeypatch): + """``bytes``' own repr would put a whole attachment in any log line. + + The released SDK stored a ``Response``, whose repr also withheld it. + """ + item_value = _fetch_attachment( + monkeypatch, b"super-secret-key-material", filename="id_rsa" + ) + + for rendered in (repr(item_value), str(item_value), f"{item_value}"): + assert "secret-key-material" not in rendered + assert "id_rsa" in rendered + assert "25 bytes" in rendered + + +def test_file_attachment_is_constructible_with_bytes_alone(): + """Both keyword arguments are optional, as any carrier should be.""" + attachment = FileAttachment(b"z") + + assert attachment == b"z" + assert attachment.encoding is None + assert attachment.filename is None + assert repr(attachment) == "" + + +def test_file_attachment_repr_escapes_a_control_character_in_a_filename(monkeypatch): + """Short enough to survive the cap, so escaping is what is under test. + + An unescaped filename would put raw ANSI into a terminal reading the log. + """ + item_value = _fetch_attachment(monkeypatch, b"z", filename="\x1b[31mboom.txt") + + rendered = repr(item_value) + assert "\x1b" not in rendered + assert "\\x1b" in rendered + assert rendered.endswith("boom.txt': 1 bytes>") + + +def test_file_attachment_repr_survives_an_unprintable_filename(): + """Only reachable by hand, but a repr that raises breaks every log call.""" + + class Hostile: + def __repr__(self): + raise RuntimeError("boom") + + attachment = FileAttachment(b"z", filename=Hostile()) + + assert repr(attachment) == ": 1 bytes>" + + +def test_file_attachment_repr_caps_a_hostile_filename(monkeypatch): + """``filename`` is server data: the one unbounded part of a bounded repr.""" + item_value = _fetch_attachment( + monkeypatch, b"file-bytes", filename="a" * 30 + "\n\x1b[31m" + "a" * 470 + "'" + ) + + rendered = repr(item_value) + assert len(rendered) < 120 + assert rendered.startswith("") + assert "\x1b" not in rendered + + +def test_file_attachment_survives_copy_and_pickle(monkeypatch): + """Both rebuild through ``__new__``, so the attributes must come back.""" + item_value = _fetch_attachment( + monkeypatch, b"file-bytes", encoding="iso-8859-1", filename="notes.txt" + ) + + for clone in ( + copy.copy(item_value), + copy.deepcopy(item_value), + pickle.loads(pickle.dumps(item_value)), + ): + assert isinstance(clone, FileAttachment) + assert clone.content == b"file-bytes" + assert clone.text == "file-bytes" + assert clone.encoding == "iso-8859-1" + assert clone.filename == "notes.txt" + + +def test_file_attachment_subclass_keeps_its_own_attributes(): + """``__getnewargs__`` passes only the bytes, so the default reduce still + carries the instance dict and a subclass is not cut down. + """ + tagged = _TaggedAttachment(b"z", tag="keepme", filename="n.bin") + + for clone in ( + copy.copy(tagged), + copy.deepcopy(tagged), + pickle.loads(pickle.dumps(tagged)), + ): + assert isinstance(clone, _TaggedAttachment) + assert clone.tag == "keepme" + assert clone.filename == "n.bin" + + +def test_file_attachment_rebuild_does_not_re_run_init(): + """Copy and pickle go through ``__new__``, never the constructor. + + Calling the class instead would re-run a subclass's ``__init__`` with + only the bytes, which the default reduce this pins never does. + """ + original = _InitAttachment(b"z", "kept", filename="n.bin") + + for clone in ( + copy.copy(original), + copy.deepcopy(original), + pickle.loads(pickle.dumps(original)), + ): + assert clone.extra == "kept" + assert clone.filename == "n.bin" + + +def test_file_attachment_survives_losing_its_own_attributes(): + """Pickle protocols 0 and 1 rebuild without ``__new__``, so the class + defaults are what keep ``.text`` from raising ``AttributeError``. + """ + attachment = FileAttachment(b"z", encoding="utf-8", filename="n.bin") + del attachment.encoding + del attachment.filename + + assert attachment.text == "z" + assert repr(attachment) == "" + + +def test_file_attachment_repr_names_the_actual_class(): + """A subclass must not be logged under the base class's name.""" + assert repr(_TaggedAttachment(b"z")) == "<_TaggedAttachment: 1 bytes>" + + +# The two bodies ``json.loads`` answers with something other than +# ``JSONDecodeError``: ``None`` gives ``TypeError``, non-UTF-8 bytes give +# ``UnicodeDecodeError``. Every reader must treat both as "not JSON". +_UNREADABLE_BODIES = [None, b'{"a": "caf\xe9"}'] + + +class _UnreadableBody: + """A response whose body no JSON reader can parse. + + ``.content`` is ``None`` when ``requests`` has no ``raw`` stream; the + other shape is a body that is not valid UTF-8. + """ + + encoding = None + + def __init__(self, status_code, content=None): + self.status_code = status_code + self.ok = status_code < 400 + self.content = content + + def json(self): + raise AssertionError("no reader may call .json() on a response body") + + +@pytest.mark.parametrize("body", _UNREADABLE_BODIES, ids=["none", "not-utf8"]) +def test_a_body_that_cannot_be_read_at_all_raises_secret_server_error( + monkeypatch, body +): + """``get_secret`` documents ``SecretServerError`` as its only failure.""" + monkeypatch.setattr(HTTP_GET, lambda *args, **kwargs: _UnreadableBody(200, body)) + server = make_server("https://ss.example.com", "secret_server") + + with pytest.raises(SecretServerError) as raised: + server.get_secret(1) + + assert "Secret endpoint did not return JSON: HTTP 200" in str(raised.value) + + +@pytest.mark.parametrize("body", _UNREADABLE_BODIES, ids=["none", "not-utf8"]) +def test_a_client_error_with_no_readable_body_raises_cleanly(monkeypatch, body): + """``process`` parses a 4xx body as JSON, so it meets the same bodies. + + A bare ``TypeError`` is not what ``:raise:`` promises the caller. + """ + monkeypatch.setattr(HTTP_GET, lambda *args, **kwargs: _UnreadableBody(403, body)) + server = make_server("https://ss.example.com", "secret_server") + + with pytest.raises(SecretServerError) as raised: + server.get_secret(1) + + assert "HTTP 403" in str(raised.value) + + +@pytest.mark.parametrize("body", _UNREADABLE_BODIES, ids=["none", "not-utf8"]) +def test_a_token_response_with_no_readable_body_raises_cleanly(monkeypatch, body): + """The token parser reads ``.content`` too, with the same two traps.""" + monkeypatch.setattr(HTTP_POST, lambda *args, **kwargs: _UnreadableBody(200, body)) + authorizer = make_grant_authorizer() + + with pytest.raises(SecretServerError) as raised: + authorizer.get_access_token() + + assert "did not return a JSON access grant" in str(raised.value) + + +def test_file_attachment_without_a_filename_still_reprs(monkeypatch): + """``filename`` is absent from the item dict for some templates.""" + item_value = _fetch_attachment(monkeypatch, b"file-bytes") + + assert item_value.filename is None + assert repr(item_value) == "" + + +def test_unfetched_file_attachment_is_left_alone(monkeypatch): + """``fetch_file_attachments=False`` must not build a carrier at all.""" + server = _attachment_server(monkeypatch, [_ONE_FILE]) + + secret = server.get_secret(1, fetch_file_attachments=False) + + assert secret["items"][0]["itemValue"] is None + + +def test_ordinary_field_values_are_not_overwritten(monkeypatch): + """The loop keys off a truthy ``fileAttachmentId``, not the key's presence. + + Every item carries the key, 0 for a field that is not a file. + """ + server = _attachment_server(monkeypatch, [_ONE_FILE]) + + secret = server.get_secret(1, fetch_file_attachments=True) + + assert secret["items"][1]["itemValue"] == "p@ss" + + +def test_each_read_parses_its_own_items(monkeypatch): + """``get_secret`` mutates what it returns, so it must not be shared. + + A second read of the same secret cannot see the first read's values. + """ + server = _attachment_server(monkeypatch, [_ONE_FILE]) + + first = server.get_secret(1, fetch_file_attachments=True) + first["items"][1]["itemValue"] = "clobbered" + second = server.get_secret(1, fetch_file_attachments=True) + + assert second["items"][1]["itemValue"] == "p@ss" + + +@pytest.mark.parametrize("slug", [None, "", 42], ids=["absent", "empty", "int"]) +def test_a_file_field_with_no_usable_slug_raises(monkeypatch, slug): + """``slug`` builds the field URL, so an unusable one cannot be fetched. + + An empty one would fetch the fields collection; indexing a missing one + would leave a ``KeyError`` where the API promises its own error. + """ + item = {"fileAttachmentId": 42, "filename": "f.txt"} + if slug is not None: + item["slug"] = slug + body = FakeResponse(json_data={"id": 7, "items": [item]}) + + monkeypatch.setattr(HTTP_GET, lambda *args, **kwargs: body) + server = make_server("https://ss.example.com", "secret_server") + + with pytest.raises(SecretServerError) as raised: + server.get_secret(1) + + assert "file field with no 'slug'" in str(raised.value) + # The secret's own response, not a field's: no field was ever fetched. + assert raised.value.response is body + + +def test_a_secret_with_no_items_is_returned_unchanged(monkeypatch): + """An empty list is a valid answer, not a malformed body.""" + monkeypatch.setattr( + HTTP_GET, lambda *a, **k: FakeResponse(json_data={"id": 7, "items": []}) + ) + server = make_server("https://ss.example.com", "secret_server") + + assert server.get_secret(1) == {"id": 7, "items": []} + + +def test_an_empty_folder_returns_no_secret_ids(monkeypatch): + """The same for ``records``: an empty folder is not a malformed body.""" + + def fake_get(url, *args, **kwargs): + if url.endswith("/secrets/search-total"): + return FakeResponse(text="0") + return FakeResponse(json_data={"records": []}) + + monkeypatch.setattr(HTTP_GET, fake_get) + server = make_server("https://ss.example.com", "secret_server") + + assert server.get_secret_ids_by_folderid(2) == [] + + +def test_an_item_without_a_file_attachment_id_is_left_alone(monkeypatch): + """Absent, not zero: the key is missing for some templates. + + Indexing it would raise ``KeyError`` out of a ``SecretServerError`` API. + """ + + def fake_get(url, *args, **kwargs): + items = [{"slug": "password", "itemValue": "p@ss"}] + return FakeResponse(json_data={"id": 7, "items": items}) + + monkeypatch.setattr(HTTP_GET, fake_get) + server = make_server("https://ss.example.com", "secret_server") + + secret = server.get_secret(1, fetch_file_attachments=True) + + assert secret["items"][0]["itemValue"] == "p@ss" + + +@pytest.mark.parametrize( + "items", + ["not-a-list", [1, 2], [{"slug": "a"}, "not-an-object"], 42], + ids=["string", "numbers", "mixed", "int"], +) +def test_a_secret_whose_items_are_not_objects_raises(monkeypatch, items): + """``_get_json`` vouches for the body; the key read out of it needs the + same, or a malformed payload escapes as whatever indexing it happens to + raise -- ``TypeError`` or ``AttributeError``, never the documented error. + """ + + body = FakeResponse(json_data={"id": 7, "items": items}) + + monkeypatch.setattr(HTTP_GET, lambda *args, **kwargs: body) + server = make_server("https://ss.example.com", "secret_server") + + with pytest.raises(SecretServerError) as raised: + server.get_secret(1) + + assert "did not return 'items' as a list of objects" in str(raised.value) + assert raised.value.response is body + + +def test_each_attachment_is_fetched_from_its_own_field(monkeypatch): + """One request per file field, each value paired with its own slug.""" + seen = [] + files = [ + ("first", b"AAA", "a.bin", None), + ("second", b"BBBB", "b.bin", None), + ] + server = _attachment_server(monkeypatch, files, seen=seen) + + items = server.get_secret(1, fetch_file_attachments=True)["items"] + + assert [item["itemValue"] for item in items[:2]] == [b"AAA", b"BBBB"] + assert [item["itemValue"].filename for item in items[:2]] == ["a.bin", "b.bin"] + assert [slug for slug, _ in seen] == ["secret", "first", "second"] + + +def test_query_params_reach_the_secret_body_and_every_field(monkeypatch): + """Both the secret body and every field fetch get the caller's params.""" + seen = [] + server = _attachment_server(monkeypatch, [_ONE_FILE], seen=seen) + + server.get_secret(1, query_params={"autoComment": "why"}) + + assert seen == [ + ("secret", {"autoComment": "why"}), + ("file-slug", {"autoComment": "why"}), + ] + + +def test_get_secret_by_path_forwards_the_path_and_the_flag(monkeypatch): + """The path travels as a query parameter, and the flag is not overridden.""" + seen = [] + server = _attachment_server(monkeypatch, [_ONE_FILE], seen=seen) + + secret = server.get_secret_by_path("/a/b/", fetch_file_attachments=False) + + assert secret["items"][0]["itemValue"] is None + assert seen == [("secret", {"secretPath": "\\a\\b"})] + + +def test_a_failing_attachment_fetch_raises(monkeypatch): + """A 4xx on one field must not be swallowed, nor stored as the file. + + Bypassing ``process`` would write the error body to disk downstream. + """ + seen = [] + files = [("first", b"AAA", None, None), ("second", b"BBBB", None, None)] + server = _attachment_server(monkeypatch, files, seen=seen, statuses={"second": 403}) + + with pytest.raises(SecretServerError): + server.get_secret(1, fetch_file_attachments=True) + + # The first field really was served, so the failure was mid-loop. + assert [slug for slug, _ in seen] == ["secret", "first", "second"] + + +# --------------------------------------------------------------------------- +# 4.6: non-numeric search-total body is rejected, not silently propagated +# --------------------------------------------------------------------------- + + +def test_non_numeric_search_total_raises(monkeypatch): + def fake_get(url, *args, **kwargs): + if url.endswith("/secrets/search-total"): + return FakeResponse(text="not-a-number") + return FakeResponse(json_data={"records": []}) + + monkeypatch.setattr(HTTP_GET, fake_get) + + server = make_server("https://ss.example.com", "secret_server") + + with pytest.raises(SecretServerError, match="non-numeric"): + server.get_secret_ids_by_folderid(1) + + +def test_numeric_search_total_still_works(monkeypatch): + def fake_get(url, *args, **kwargs): + if url.endswith("/secrets/search-total"): + return FakeResponse(text="2") + return FakeResponse(json_data={"records": [{"id": 1}, {"id": 2}]}) + + monkeypatch.setattr(HTTP_GET, fake_get) + + server = make_server("https://ss.example.com", "secret_server") + + assert server.get_secret_ids_by_folderid(1) == [1, 2] + + +# --------------------------------------------------------------------------- +# Review step 1: authorizers stay picklable / deep-copyable with the lock +# --------------------------------------------------------------------------- + + +def _grant_authorizer(): + return make_grant_authorizer(password="SuperSecret123") + + +def test_password_grant_authorizer_deep_copies_with_its_own_lock(): + import copy + + auth = _grant_authorizer() + clone = copy.deepcopy(auth) + + assert clone is not auth + assert clone.base_url == auth.base_url + assert clone._server_type == "secret_server" + assert clone.password == auth.password + assert clone._refresh_lock is not auth._refresh_lock + + +def test_password_grant_authorizer_shallow_copies_with_its_own_lock(): + import copy + + auth = _grant_authorizer() + clone = copy.copy(auth) + + assert clone is not auth + assert clone.username == auth.username + assert clone._refresh_lock is not auth._refresh_lock + + +@pytest.mark.parametrize("copier", ["copy", "deepcopy"]) +def test_copy_does_not_wait_for_an_in_progress_refresh(copier): + """A copy must not take ``_refresh_lock``: it would block behind a detection + plus token request, and deadlock when made from code already under the lock. + Taken mid-refresh, the clone carries no half-written grant. + """ + import copy + + auth = _grant_authorizer() + in_grant = threading.Event() + release = threading.Event() + + held = {} + + def slow_grant(token_url, grant_request): + in_grant.set() + held["released_in_time"] = release.wait(timeout=10) + return {"access_token": "orig-tok", "expires_in": 1200} + + auth.get_access_grant = slow_grant + refresher = threading.Thread(target=auth.get_access_token, daemon=True) + refresher.start() + assert in_grant.wait(timeout=10) + try: + clone = getattr(copy, copier)(auth) + # The copy must return while the refresh still holds the lock. One + # that took the lock would arrive here only after slow_grant's wait + # gave up, then pass everything below; this assertion catches that. + assert refresher.is_alive(), "copy returned only after the refresh ended" + assert "released_in_time" not in held + finally: + release.set() + join_all([refresher]) + assert held["released_in_time"] is True + + assert not hasattr(clone, "access_grant") + assert not hasattr(clone, "access_grant_refreshed") + clone.get_access_grant = lambda *a, **k: { + "access_token": "clone-tok", + "expires_in": 1200, + } + assert clone.get_access_token() == "clone-tok" + assert auth.get_access_token() == "orig-tok" + + +def test_refresh_publishes_through_ordinary_attribute_assignment(): + """A subclass may turn ``access_grant`` into a slot or a property; the + grant must reach it. Publishing through ``__dict__`` bypassed both.""" + + seen = [] + + class Observing(PasswordGrantAuthorizer): + @property + def access_grant(self): + try: + return self.__dict__["_grant"] + except KeyError: # behave like an unset attribute before first refresh + raise AttributeError("access_grant") from None + + @access_grant.setter + def access_grant(self, value): + if value is not None: # tolerate a future None-initialising __init__ + seen.append(value["access_token"]) + self.__dict__["_grant"] = value + + class Slotted(PasswordGrantAuthorizer): + __slots__ = ("access_grant",) + + for cls in (Observing, Slotted): + auth = cls("https://ss.example.com", "u", "p", server_type="secret_server") + auth.get_access_grant = lambda *a, **k: { + "access_token": "tok", + "expires_in": 1200, + } + assert auth.get_access_token() == "tok" + assert auth.access_grant["access_token"] == "tok" + assert seen == ["tok"] + + +def test_copy_from_inside_a_refresh_callback_does_not_deadlock(): + """An overridden ``get_access_grant`` (or a framework deep-copying an + object graph from one) runs under ``_refresh_lock``; copying the + authorizer there must return, not hang the thread forever.""" + import copy + + auth = _grant_authorizer() + seen = {} + + def copying_grant(token_url, grant_request): + seen["shallow"] = copy.copy(auth) + seen["deep"] = copy.deepcopy({"auth": auth, "n": 1})["auth"] + return {"access_token": "tok", "expires_in": 1200} + + auth.get_access_grant = copying_grant + result = [] + worker = threading.Thread( + target=lambda: result.append(auth.get_access_token()), daemon=True + ) + worker.start() + join_all([worker], timeout=5) # fails, instead of hanging, on a deadlock + assert result == ["tok"] + assert seen["shallow"]._refresh_lock is not auth._refresh_lock + assert seen["deep"]._refresh_lock is not auth._refresh_lock + + +@pytest.mark.parametrize("present", ["access_grant", "access_grant_refreshed"]) +def test_copy_drops_a_half_written_grant_pair(present): + """``_refresh`` writes the grant, then its timestamp. A snapshot taken + between the two must not produce a clone that raises AttributeError + on every call; the incomplete pair is dropped and the clone refreshes.""" + import copy + + auth = _grant_authorizer() + # Reproduce the half-written state directly; the real window is one + # bytecode wide and cannot be hit deterministically from a test. + if present == "access_grant": + auth.__dict__["access_grant"] = {"access_token": "orphan", "expires_in": 1200} + else: + auth.__dict__["access_grant_refreshed"] = datetime.now(timezone.utc) + + clone = copy.copy(auth) + assert not hasattr(clone, "access_grant") + assert not hasattr(clone, "access_grant_refreshed") + clone.get_access_grant = lambda *a, **k: { + "access_token": "fresh", + "expires_in": 1200, + } + assert clone.get_access_token() == "fresh" + # The original is left exactly as it was. + assert present in auth.__dict__ + + +def test_deep_copied_authorizer_refreshes_independently(): + """The copy has its own grant state and lock; refreshing it must neither + require nor disturb the original.""" + import copy + + auth = _grant_authorizer() + clone = copy.deepcopy(auth) + clone.get_access_grant = lambda token_url, grant_request: { + "access_token": "clone-tok", + "expires_in": 1200, + } + + assert clone.get_access_token() == "clone-tok" + assert not hasattr(auth, "access_grant") + + +def test_deepcopy_of_container_holding_authorizer_preserves_identity_semantics(): + """``memo`` bookkeeping: the same authorizer referenced twice in one + structure deep-copies to a single clone, as for any other object.""" + import copy + + auth = _grant_authorizer() + pair = copy.deepcopy([auth, auth]) + + assert pair[0] is pair[1] + assert pair[0] is not auth + + +def test_password_grant_authorizer_refuses_to_pickle(): + """A pickle leaves the process carrying the plaintext password, so it is + refused with an actionable error. This replaces an accidental ``TypeError: + cannot pickle '_thread.lock'`` that also broke ``copy.deepcopy``. + """ + import pickle + + auth = _grant_authorizer() + with pytest.raises(TypeError, match="holds live credentials") as excinfo: + pickle.dumps(auth) + assert "copy.deepcopy" in str(excinfo.value) + + +def test_access_token_authorizer_refuses_to_pickle(): + """The same policy for the other credential holder: a pre-issued bearer + token must not be written to a disk cache or a worker pipe either.""" + import copy + import pickle + + auth = AccessTokenAuthorizer( + "bearer-secret-token", "https://ss.example.com", server_type="secret_server" + ) + for protocol in range(pickle.HIGHEST_PROTOCOL + 1): + with pytest.raises(TypeError, match="live bearer token"): + pickle.dumps(auth, protocol=protocol) + # In-memory copies still work: there is no lock to worry about here. + assert copy.copy(auth).get_access_token() == "bearer-secret-token" + assert copy.deepcopy(auth).get_access_token() == "bearer-secret-token" + + +def _response_for(method, url, **kwargs): + """A real ``requests.Response`` with a real ``PreparedRequest``. + + Built locally: preparing a request issues no I/O, so this stays offline + while reproducing exactly what the SDK attaches to an error. + """ + response = requests.Response() + response.status_code = 400 + response._content = b'{"error":"invalid_grant"}' + response.request = requests.Request(method, url, **kwargs).prepare() + return response + + +PASSWORD = "pickle-probe-password" +BEARER = "pickle-probe-bearer-token" + + +@pytest.mark.parametrize("error_type", [SecretServerError, SecretServerClientError]) +def test_pickled_error_carries_no_grant_credentials(error_type): + """The token-endpoint response holds the grant as its request body, so + pickling an error that kept it would write the password wherever the + pickle goes. A process pool does that unasked, to propagate a failure. + """ + import pickle + + response = _response_for( + "POST", + "https://ss.example.com/oauth2/token", + data={"username": "svc", "password": PASSWORD, "grant_type": "password"}, + ) + assert PASSWORD in response.request.body # the leak exists to be stopped + + error = error_type("Token endpoint rejected the grant", response) + blob = pickle.dumps(error) + assert PASSWORD.encode() not in blob + assert b"oauth2/token" not in blob + + revived = pickle.loads(blob) + assert type(revived) is error_type + assert revived.message == "Token endpoint rejected the grant" + assert str(revived) == str(error) + assert revived.response is None + # In-memory use is untouched: ``.response`` is documented API. + assert error.response is response + assert error.response.status_code == 400 + + +def test_pickled_error_carries_no_bearer_token(): + """Every API error attaches a response whose request carries the + Authorization header.""" + import pickle + + response = _response_for( + "GET", + "https://ss.example.com/api/v1/secrets/1", + headers={"Authorization": f"Bearer {BEARER}"}, + ) + assert BEARER in response.request.headers["Authorization"] + error = SecretServerError("HTTP 400: bad request", response) + assert BEARER.encode() not in pickle.dumps(error) + + +def test_shared_failure_still_rebuilds_with_its_response(): + """``_shared_failure`` reconstructs an error as ``type(e)(message, + response)``. The pickle change must not disturb that constructor. + """ + original = SecretServerClientError( + "client boom", _response_for("GET", "https://ss.example.com/api/v1/x") + ) + shared = Authorizer._shared_failure(original) + assert type(shared) is SecretServerClientError + assert shared.message == original.message + assert shared.response is original.response + + +def test_pickle_refusal_never_emits_the_password(): + """Belt and braces: no pickle protocol may produce bytes for this object.""" + import pickle + + auth = _grant_authorizer() + for protocol in range(pickle.HIGHEST_PROTOCOL + 1): + with pytest.raises(TypeError): + pickle.dumps(auth, protocol=protocol) + + +def test_domain_authorizer_inherits_copy_and_pickle_behaviour(): + import copy + import pickle + + from delinea.secrets.server import DomainPasswordGrantAuthorizer + + auth = DomainPasswordGrantAuthorizer( + "https://ss.example.com", + "user", + "example.com", + "pass", + server_type="secret_server", + ) + clone = copy.deepcopy(auth) + assert clone.domain == "example.com" + assert clone._refresh_lock is not auth._refresh_lock + with pytest.raises(TypeError, match="DomainPasswordGrantAuthorizer"): + pickle.dumps(auth) + + +# --------------------------------------------------------------------------- +# Review step 1: get_folder_json accepts every params form requests accepts +# --------------------------------------------------------------------------- + + +def _folder_server(monkeypatch, calls): + """Records ``(url, params)`` for every GET.""" + + def fake_get(url, *args, **kwargs): + calls.append((url, kwargs.get("params"))) + return FakeResponse(json_data={"id": 1}) + + monkeypatch.setattr(HTTP_GET, fake_get) + return make_server("https://ss.example.com", "secret_server") + + +@pytest.mark.parametrize( + "params", + ["take=5", b"take=5", [("take", "5")], {"take": 5}], +) +def test_get_folder_json_accepts_any_params_form(monkeypatch, params): + """A mapping stays a mapping; every other form becomes a list of pairs, so + repeated keys survive. Either way the flag is sent exactly once. The old + ``dict()`` coercion raised ValueError on a query string or pairs.""" + calls = [] + server = _folder_server(monkeypatch, calls) + server.get_folder_json(1, query_params=params) + url, sent = calls[-1] + assert url.endswith("/folders/1") + as_dict = sent if isinstance(sent, dict) else dict(sent) + assert as_dict["getAllChildren"] == "true" + assert str(as_dict["take"]) == "5" + assert len(sent) == 2 # no duplicate key in either form + + +def test_get_folder_json_does_not_mutate_caller_params(monkeypatch): + calls = [] + server = _folder_server(monkeypatch, calls) + params = {"take": 5} + server.get_folder_json(1, query_params=params) + assert params == {"take": 5} + + +def test_get_folder_json_string_params_passthrough_without_children(monkeypatch): + calls = [] + server = _folder_server(monkeypatch, calls) + server.get_folder_json(1, query_params="take=5", get_all_children=False) + url, sent = calls[-1] + assert url.endswith("/folders/1") + assert sent == "take=5" + + +# --------------------------------------------------------------------------- +# Review step 2: non-JSON folder lookup body is excerpted, not echoed +# --------------------------------------------------------------------------- + + +def test_child_folder_lookup_non_json_is_excerpted(monkeypatch): + responses = [ + FakeResponse(json_data={"total": 3}), + FakeResponse(text="" + "x" * 500), + ] + + def fake_get(url, *args, **kwargs): + return responses.pop(0) + + monkeypatch.setattr(HTTP_GET, fake_get) + server = make_server("https://ss.example.com", "secret_server") + + with pytest.raises(SecretServerError) as excinfo: + server.get_child_folder_ids_by_folderid(7) + err = excinfo.value + assert err.message.startswith("Folder lookup did not return JSON: HTTP 200: ") + assert err.message.endswith("...[truncated]") + assert len(err.message) < 300 + + +# --------------------------------------------------------------------------- +# Review step 4: one request helper, one access token per API call +# --------------------------------------------------------------------------- + + +def _grant_server(monkeypatch, fake_get, server_type, base_url): + """A SecretServer over a PasswordGrantAuthorizer, counting token POSTs. + + Counting POSTs measures how often the password is sent, not how often + ``get_access_token()`` is called, which is free while the grant is valid. + """ + posts = {"count": 0} + + def counting_post(url, *args, **kwargs): + posts["count"] += 1 + return fake_token_post(url, *args, **kwargs) + + monkeypatch.setattr(HTTP_GET, fake_get) + monkeypatch.setattr(HTTP_POST, counting_post) + authorizer = PasswordGrantAuthorizer( + base_url, "user", "pass", server_type=server_type + ) + return SecretServer(base_url, authorizer), posts + + +def test_platform_calls_reuse_one_token_grant(monkeypatch): + """Across the vault-broker lookup and two API calls the password is sent + to the token endpoint exactly once.""" + + def fake_get(url, *args, **kwargs): + if "vaultbroker" in url: + return vault_broker_response() + return FakeResponse(json_data={"id": 1}) + + server, posts = _grant_server( + monkeypatch, fake_get, "platform", "https://platform.example.com" + ) + + server.get_secret_json(1) + server.get_secret_json(2) + assert posts["count"] == 1 + assert server.base_url == "https://vault.example.com" + + +def test_attachment_burst_reuses_one_token_grant(monkeypatch): + """Each attachment rebuilds headers so a refresh can happen mid-burst if + one is due, but with a valid grant that costs no token POST at all.""" + secret_body = json.dumps( + { + "items": [ + {"fileAttachmentId": 11, "slug": "a", "itemValue": None}, + {"fileAttachmentId": 12, "slug": "b", "itemValue": None}, + {"fileAttachmentId": 13, "slug": "c", "itemValue": None}, + ] + } + ) + + def fake_get(url, *args, **kwargs): + if "/fields/" in url: + return AttachmentResponse(b"file-contents") + return FakeResponse(text=secret_body) + + server, posts = _grant_server( + monkeypatch, fake_get, "secret_server", "https://ss.example.com" + ) + + secret = server.get_secret(1) + assert [item["itemValue"] for item in secret["items"]] == [b"file-contents"] * 3 + assert posts["count"] == 1 + + +def test_attachment_fetch_refreshes_an_expired_grant_mid_burst(monkeypatch): + """The point of per-attachment headers: a grant that expires between + attachments is refreshed, not sent expired to fail with 401.""" + from datetime import timedelta + + secret_body = json.dumps( + { + "items": [ + {"fileAttachmentId": 11, "slug": "a", "itemValue": None}, + {"fileAttachmentId": 12, "slug": "b", "itemValue": None}, + ] + } + ) + tokens_seen = [] + + def fake_get(url, *args, **kwargs): + if "/fields/" in url: + tokens_seen.append(kwargs["headers"]["Authorization"]) + if url.endswith("/fields/a"): + # Expire the grant on the server's clock between attachments. + server.authorizer.access_grant_refreshed -= timedelta(hours=1) + return AttachmentResponse(b"file-contents") + return FakeResponse(text=secret_body) + + server, posts = _grant_server( + monkeypatch, fake_get, "secret_server", "https://ss.example.com" + ) + + server.get_secret(1) + # One grant for the secret + first attachment, a fresh one for the second. + assert posts["count"] == 2 + assert len(tokens_seen) == 2 + + +def test_ensure_vault_url_resolves_lazy_detection_itself(monkeypatch): + """Called directly, before any API call, ``ensure_vault_url`` must still + switch to the vault URL for a PasswordGrantAuthorizer that has not yet + detected its server type -- not silently do nothing.""" + + def fake_get(url, *args, **kwargs): + if "vaultbroker" in url: + return vault_broker_response("https://vault.example.com") + # Health probes: platform is healthy, Secret Server is not. + return health_response(url.endswith("/health")) + + monkeypatch.setattr(HTTP_GET, fake_get) + monkeypatch.setattr(HTTP_POST, fake_token_post) + + authorizer = PasswordGrantAuthorizer("https://platform.example.com", "user", "pass") + server = SecretServer("https://platform.example.com", authorizer) + assert not hasattr(authorizer, "_server_type") + + server.ensure_vault_url() + assert authorizer._server_type == "platform" + assert server.base_url == "https://vault.example.com" + + +def test_ensure_vault_url_is_a_no_op_after_the_first_resolution(monkeypatch): + gets = [] + + def fake_get(url, *args, **kwargs): + gets.append(url) + return FakeResponse(json_data={"id": 1}) + + monkeypatch.setattr(HTTP_GET, fake_get) + server = make_server("https://ss.example.com", "secret_server") + + server.ensure_vault_url() + server.ensure_vault_url() + server.get_secret_json(1) + # No vault-broker call for Secret Server, and the API call still went out. + assert gets == ["https://ss.example.com/api/v1/secrets/1"] + + +@pytest.mark.parametrize( + "call,expected_params", + [ + (lambda s: s.search_secrets(), None), + (lambda s: s.search_secrets(query_params={"a": "b"}), {"a": "b"}), + (lambda s: s.lookup_folders(), None), + (lambda s: s.lookup_folders(query_params={"a": "b"}), {"a": "b"}), + (lambda s: s.get_secret_json(1), None), + (lambda s: s.get_secret_json(1, query_params={"a": "b"}), {"a": "b"}), + ], +) +def test_read_paths_pass_params_through_unchanged(monkeypatch, call, expected_params): + """Collapsing the ``if query_params is None`` twin branches into a single + call must not change what reaches ``requests``.""" + seen = [] + + def fake_get(url, *args, **kwargs): + seen.append(kwargs.get("params")) + return FakeResponse(json_data={"records": []}) + + monkeypatch.setattr(HTTP_GET, fake_get) + server = make_server("https://ss.example.com", "secret_server") + + call(server) + assert seen[-1] == expected_params + + +def test_read_paths_target_the_same_urls_as_before(monkeypatch): + """``_get`` joins the path under ``api_url`` exactly as the inlined + f-strings did.""" + seen = [] + + def fake_get(url, *args, **kwargs): + seen.append(url) + return FakeResponse(json_data={"total": 0, "records": []}) + + monkeypatch.setattr(HTTP_GET, fake_get) + server = make_server("https://ss.example.com", "secret_server") + api = "https://ss.example.com/api/v1" + + server.get_secret_json(5) + server.get_folder_json(6, get_all_children=False) + server.search_secrets() + server.lookup_folders() + server.get_child_folder_ids_by_folderid(9) + + assert seen == [ + f"{api}/secrets/5", + f"{api}/folders/6", + f"{api}/secrets", + f"{api}/folders/lookup", + f"{api}/folders/lookup", + ] + + +def test_get_folder_json_flag_wins_over_caller_getallchildren(monkeypatch): + """Carrying the flag in the URL sent the key twice when the caller also + passed it; the flag must win and appear once, as on main.""" + calls = [] + server = _folder_server(monkeypatch, calls) + caller = {"getAllChildren": "false", "take": 1} + server.get_folder_json(1, query_params=caller) + url, sent = calls[-1] + assert "getAllChildren" not in url + assert sent == {"getAllChildren": "true", "take": 1} + assert caller == {"getAllChildren": "false", "take": 1} + + +@pytest.mark.parametrize( + "body", + [ + FakeResponse(text="blocked"), + FakeResponse(json_data=[]), + FakeResponse(json_data={"count": 1}), + ], +) +def test_child_folder_total_shape_errors_are_secret_server_errors(monkeypatch, body): + """Every shape a folder lookup can come back in is a SecretServerError.""" + monkeypatch.setattr(HTTP_GET, lambda *a, **k: body) + server = make_server("https://ss.example.com", "secret_server") + with pytest.raises(SecretServerError, match="Folder lookup did not return"): + server.get_child_folder_ids_by_folderid(7) + + +# --------------------------------------------------------------------------- +# Round 9: refresh fast path, legacy hooks, one warning per wrapper +# --------------------------------------------------------------------------- + + +def test_fresh_grant_is_used_without_taking_the_refresh_lock(): + """A thread holding a valid token must not wait behind another thread's + token request. The lock is held by the test; the call must still return.""" + auth = make_grant_authorizer() + auth.access_grant = {"access_token": "still-good", "expires_in": 1200} + auth.access_grant_refreshed = datetime.now(timezone.utc) + got = [] + assert auth._refresh_lock.acquire(timeout=1) + try: + worker = threading.Thread( + target=lambda: got.append(auth.get_access_token()), daemon=True + ) + worker.start() + join_all([worker], timeout=2) + finally: + auth._refresh_lock.release() + assert got == ["still-good"] + + +def test_refresh_with_a_stale_grant_still_serialises_behind_the_lock(): + """The fast path applies only to a fresh grant; a stale one takes the lock + so there is still exactly one refresher.""" + auth = make_grant_authorizer() + auth.access_grant = {"access_token": "expired", "expires_in": 1200} + auth.access_grant_refreshed = datetime.now(timezone.utc) - timedelta(seconds=5000) + auth.get_access_grant = lambda *a, **k: {"access_token": "new", "expires_in": 1200} + assert auth._refresh_lock.acquire(timeout=1) + try: + worker = threading.Thread(target=auth.get_access_token, daemon=True) + worker.start() + worker.join(0.3) + assert worker.is_alive(), "a stale grant must wait for the refresh lock" + finally: + auth._refresh_lock.release() + join_all([worker], timeout=2) + assert auth.get_access_token() == "new" + + +def test_subclass_overriding_the_one_argument_detection_hook_still_works(): + """Before ``server_type`` existed, overriding ``_perform_server_detection`` + was the only way to skip the probes; that override must keep constructing.""" + + class NoProbe(AccessTokenAuthorizer): + def _perform_server_detection(self, base_url): + self._server_type = "platform" + + assert NoProbe("tok", "https://x.example.com")._server_type == "platform" + + +def test_legacy_wrapper_emits_one_insecure_warning_even_under_always(): + """``SecretServerV0`` builds an authorizer and a client for one URL; only + one of them may warn, or ``-W always`` shows the same line twice.""" + with warnings.catch_warnings(record=True) as record: + warnings.simplefilter("always") + SecretServerV0( + "http://legacy.example.com", "u", "p", server_type="secret_server" + ) + insecure = [w for w in record if "does not use https" in str(w.message)] + assert len(insecure) == 1 + + +def test_client_still_warns_for_its_own_insecure_url(): + """Suppression applies only when the authorizer already covered the same + URL; a different insecure client URL is still reported.""" + with warnings.catch_warnings(record=True) as record: + warnings.simplefilter("always") + authorizer = AccessTokenAuthorizer( + "tok", "http://auth.example.com", server_type="platform" + ) + SecretServer("http://api.example.com", authorizer) + insecure = [ + str(w.message) for w in record if "does not use https" in str(w.message) + ] + assert len(insecure) == 2 diff --git a/tests/test_server_detection_cache.py b/tests/test_server_detection_cache.py new file mode 100644 index 0000000..59a4b69 --- /dev/null +++ b/tests/test_server_detection_cache.py @@ -0,0 +1,833 @@ +"""Offline unit tests for the process-scoped server-detection cache. + +The network is mocked by patching ``delinea.secrets.server.requests.get``, so +no live credentials are needed. ``clear_detection_cache`` isolates the cache. +""" + +import threading + +import pytest + +from delinea.secrets.server import ( + _DETECTION_WAIT_TIMEOUT, + DEFAULT_REQUEST_TIMEOUT, + AccessTokenAuthorizer, + Authorizer, + PasswordGrantAuthorizer, + SecretServerError, +) +from fakes import ( + HTTP_GET, + HTTP_POST, + TOKEN_FROM_FAKE_ENDPOINT, + HostileBody, + fake_token_post, + health_response, + join_all, +) + +# Shared fixtures from tests/conftest.py: fail loudly on an unmocked HTTP +# call, and isolate the process-global server-detection cache. +pytestmark = pytest.mark.usefixtures("no_network", "clear_detection_cache") + +SECRET_SERVER_HEALTH = "/api/v1/healthcheck" +PLATFORM_HEALTH = "/health" + + +def make_probe_counter(healthy_endpoints): + """Return a (fake_get, counter) pair replacing ``requests.get``. + + ``fake_get`` answers healthy only for a URL ending in one of + ``healthy_endpoints``; ``counter`` tracks probes per endpoint and in total. + """ + + # "rounds" counts probe sequences that began, i.e. hits on the FIRST + # endpoint of the pair. A platform detection issues two GETs per round and + # a cache hit none, so "rounds" is the "probe pair fired N times" metric. + counter = {"total": 0, "rounds": 0, SECRET_SERVER_HEALTH: 0, PLATFORM_HEALTH: 0} + + def fake_get(url, *args, **kwargs): + for suffix in (SECRET_SERVER_HEALTH, PLATFORM_HEALTH): + if url.endswith(suffix): + counter["total"] += 1 + counter[suffix] += 1 + if suffix == SECRET_SERVER_HEALTH: + counter["rounds"] += 1 + return health_response(suffix in healthy_endpoints) + # Any other GET (e.g. vault lookups) is not a health probe. + return health_response(False) + + return fake_get, counter + + +# Behavior 1: repeated construction with the same base_url probes once total. +def test_repeated_construction_probes_once(monkeypatch): + base_url = "https://platform.example.com" + fake_get, counter = make_probe_counter({PLATFORM_HEALTH}) + monkeypatch.setattr(HTTP_GET, fake_get) + + instances = [AccessTokenAuthorizer("tok", base_url) for _ in range(20)] + + assert all(inst._server_type == "platform" for inst in instances) + # The probe pair fires exactly once total across all 20 constructions. + assert counter["rounds"] == 1 + assert counter[PLATFORM_HEALTH] == 1 + assert counter[SECRET_SERVER_HEALTH] == 1 + + +# Behavior 2: cache is shared across different authorizer subclasses. +def test_cache_shared_across_subclasses(monkeypatch): + base_url = "https://platform.example.com" + fake_get, counter = make_probe_counter({PLATFORM_HEALTH}) + monkeypatch.setattr(HTTP_GET, fake_get) + + monkeypatch.setattr(HTTP_POST, fake_token_post) + + AccessTokenAuthorizer("tok", base_url) + grant = PasswordGrantAuthorizer(base_url, "user", "pass") + # Triggers lazy detection in _refresh, which must reuse the cached result. + assert grant.get_access_token() == TOKEN_FROM_FAKE_ENDPOINT + + assert grant._server_type == "platform" + # Detection probes fire once total across both authorizers. + assert counter["rounds"] == 1 + + +# Behavior 3: a cache hit still sets the per-instance _server_type attribute. +def test_cache_hit_sets_instance_attr(monkeypatch): + base_url = "https://platform.example.com" + fake_get, counter = make_probe_counter({PLATFORM_HEALTH}) + monkeypatch.setattr(HTTP_GET, fake_get) + + AccessTokenAuthorizer("tok", base_url) # populates the cache + assert counter["rounds"] == 1 + probes_after_first = counter["total"] + + second = AccessTokenAuthorizer("tok", base_url) # cache hit, no new probe + assert second._server_type == "platform" + assert counter["rounds"] == 1 + assert counter["total"] == probes_after_first + + +# Behavior 4: two distinct base_urls get independent, correct cache entries. +def test_two_distinct_base_urls(monkeypatch): + ss_url = "https://secretserver.example.com" + platform_url = "https://platform.example.com" + + def fake_get(url, *args, **kwargs): + if url.startswith(ss_url) and url.endswith(SECRET_SERVER_HEALTH): + return health_response(True) + if url.startswith(platform_url) and url.endswith(PLATFORM_HEALTH): + return health_response(True) + return health_response(False) + + monkeypatch.setattr(HTTP_GET, fake_get) + + ss_auth = AccessTokenAuthorizer("tok", ss_url) + platform_auth = AccessTokenAuthorizer("tok", platform_url) + + assert ss_auth._server_type == "secret_server" + assert platform_auth._server_type == "platform" + + cache = Authorizer._server_type_cache + assert cache[ss_url] == "secret_server" + assert cache[platform_url] == "platform" + assert len(cache) == 2 + + +# Behavior 5: detection failure is NOT cached; a later healthy probe succeeds. +def test_failure_is_not_cached(monkeypatch): + base_url = "https://unknown.example.com" + + # First: both probes unhealthy -> detection raises. + unhealthy_get, _ = make_probe_counter(set()) + monkeypatch.setattr(HTTP_GET, unhealthy_get) + with pytest.raises(SecretServerError): + AccessTokenAuthorizer("tok", base_url) + + assert base_url not in Authorizer._server_type_cache + + # Then: probes become healthy -> re-probe succeeds (failure was not cached). + healthy_get, counter = make_probe_counter({PLATFORM_HEALTH}) + monkeypatch.setattr(HTTP_GET, healthy_get) + instance = AccessTokenAuthorizer("tok", base_url) + + assert instance._server_type == "platform" + assert counter["total"] >= 1 + + +# Behavior 6: concurrent construction is thread-safe and probes few times. +def test_concurrent_construction_thread_safe(monkeypatch): + base_url = "https://platform.example.com" + fake_get, counter = make_probe_counter({PLATFORM_HEALTH}) + monkeypatch.setattr(HTTP_GET, fake_get) + + results = [] + errors = [] + start = threading.Event() + + def worker(): + start.wait() + try: + inst = AccessTokenAuthorizer("tok", base_url) + results.append(inst._server_type) + except Exception as exc: # pragma: no cover - failure path + errors.append(exc) + + threads = [threading.Thread(target=worker, daemon=True) for _ in range(20)] + for t in threads: + t.start() + start.set() + join_all(threads) + + assert errors == [] + assert len(results) == 20 + assert all(r == "platform" for r in results) + # No probe-count assertion on purpose: with an instantaneous fake probe a + # count of one cannot fail even without single-flight. That property is + # pinned deterministically by ``test_only_one_probe_is_ever_in_flight``. + + +# Behavior 7: an explicit server_type override skips detection entirely (no probe) +# and is per-instance only -- it must NOT seed the shared process cache. +@pytest.mark.parametrize("server_type", ["platform", "secret_server"]) +def test_explicit_server_type_skips_probe(monkeypatch, server_type): + base_url = "https://anything.example.com" + # Every health endpoint is unhealthy: if any probe fired, detection would + # raise. It must not, because the override bypasses probing. + fake_get, counter = make_probe_counter(set()) + monkeypatch.setattr(HTTP_GET, fake_get) + + inst = AccessTokenAuthorizer("tok", base_url, server_type=server_type) + + assert inst._server_type == server_type + assert counter["total"] == 0 # zero probes -> no WAF burst + # The unverified override must NOT be written to the shared cache (otherwise + # it could poison auto-detection for other callers using the same base_url). + assert base_url not in Authorizer._server_type_cache + + +# Behavior 8: the override is normalized (case/whitespace-insensitive). +def test_explicit_server_type_is_normalized(monkeypatch): + fake_get, counter = make_probe_counter(set()) + monkeypatch.setattr(HTTP_GET, fake_get) + + inst = AccessTokenAuthorizer( + "tok", "https://x.example.com", server_type=" Platform " + ) + + assert inst._server_type == "platform" + assert counter["total"] == 0 + + +# Behavior 9: an invalid override raises and issues no probe. +def test_invalid_server_type_raises(monkeypatch): + fake_get, counter = make_probe_counter({PLATFORM_HEALTH}) + monkeypatch.setattr(HTTP_GET, fake_get) + + with pytest.raises(SecretServerError): + AccessTokenAuthorizer("tok", "https://x.example.com", server_type="bogus") + + assert counter["total"] == 0 + + +# Behavior 10: PasswordGrantAuthorizer with an override never probes in _refresh. +def test_password_grant_override_skips_detection(monkeypatch): + base_url = "https://platform.example.com" + fake_get, counter = make_probe_counter(set()) + monkeypatch.setattr(HTTP_GET, fake_get) + + grant = PasswordGrantAuthorizer(base_url, "user", "pass", server_type="platform") + assert grant._server_type == "platform" + + monkeypatch.setattr(HTTP_POST, fake_token_post) + assert grant.get_access_token() == TOKEN_FROM_FAKE_ENDPOINT + + # The platform token endpoint was selected without any health probe. + assert counter["total"] == 0 + # Platform token endpoint was selected without any health probe. + assert grant.token_path_uri == PasswordGrantAuthorizer.PLATFORM_TOKEN_PATH_URI + + +# Behavior 11: the cache is bounded; the least-recently-used entry is evicted. +def test_cache_is_bounded_lru(monkeypatch): + # Every base_url detects as platform (healthy /health) so each distinct URL + # seeds one verified cache entry. Only verified detections populate the + # shared cache, so the cache must be filled via detection (not overrides). + fake_get, _ = make_probe_counter({PLATFORM_HEALTH}) + monkeypatch.setattr(HTTP_GET, fake_get) + + maxsize = Authorizer._SERVER_TYPE_CACHE_MAXSIZE + + # Fill exactly to capacity via auto-detection. + for i in range(maxsize): + AccessTokenAuthorizer("tok", f"https://host-{i}.example.com") + assert len(Authorizer._server_type_cache) == maxsize + + first_key = "https://host-0.example.com" + # Touch host-0 so it becomes most-recently-used and survives the next insert. + cached, _flight, _is_leader = Authorizer._start_or_join_detection(first_key) + assert cached == "platform" + + # One more distinct URL overflows the cache by one entry. + AccessTokenAuthorizer("tok", "https://overflow.example.com") + + assert len(Authorizer._server_type_cache) == maxsize + assert first_key in Authorizer._server_type_cache # survived (recently used) + assert "https://host-1.example.com" not in Authorizer._server_type_cache # evicted + + +# Behavior 13: an unverified override must not poison auto-detection for a later +# caller that relies on probing for the same base_url. +def test_override_does_not_poison_autodetect(monkeypatch): + base_url = "https://platform.example.com" + # The server is really a platform (healthy /health); probing would detect it. + fake_get, counter = make_probe_counter({PLATFORM_HEALTH}) + monkeypatch.setattr(HTTP_GET, fake_get) + + # First caller supplies a WRONG override and issues no probe. + poisoner = AccessTokenAuthorizer("tok", base_url, server_type="secret_server") + assert poisoner._server_type == "secret_server" + assert counter["total"] == 0 + assert base_url not in Authorizer._server_type_cache # not seeded + + # Second caller relies on auto-detection -> must probe and get the real type, + # NOT the poisoned override value. + detected = AccessTokenAuthorizer("tok", base_url) + assert detected._server_type == "platform" + assert counter["rounds"] == 1 # a real probe fired + assert Authorizer._server_type_cache[base_url] == "platform" + + +# Behavior 12: the public clear-cache method forces re-detection. +def test_public_clear_cache(monkeypatch): + base_url = "https://platform.example.com" + fake_get, counter = make_probe_counter({PLATFORM_HEALTH}) + monkeypatch.setattr(HTTP_GET, fake_get) + + AccessTokenAuthorizer("tok", base_url) + assert counter["rounds"] == 1 + + Authorizer.clear_server_type_cache() + assert base_url not in Authorizer._server_type_cache + + AccessTokenAuthorizer("tok", base_url) # cache empty -> probes again + assert counter["rounds"] == 2 + + +# --------------------------------------------------------------------------- +# Review step 3: single-flight detection and one owner of the cache bound +# --------------------------------------------------------------------------- + + +def test_concurrent_distinct_urls_each_probe_once(monkeypatch): + """The detection lock is per base_url, so unrelated URLs are not + serialized into a single probe (nor probed once per thread).""" + urls = ["https://one.example.com", "https://two.example.com"] + fake_get, counter = make_probe_counter({PLATFORM_HEALTH}) + monkeypatch.setattr(HTTP_GET, fake_get) + + results = [] + errors = [] + start = threading.Event() + + def worker(base_url): + def run(): + start.wait() + try: + results.append(AccessTokenAuthorizer("tok", base_url)._server_type) + except Exception as exc: # pragma: no cover - failure path + errors.append(exc) + + return run + + threads = [ + threading.Thread(target=worker(urls[i % 2]), daemon=True) for i in range(20) + ] + for t in threads: + t.start() + start.set() + join_all(threads) + + assert errors == [] + assert len(results) == 20 + assert all(r == "platform" for r in results) + # Both URLs were detected (a lower bound that can fail); the upper bound + # -- not one pair per thread -- is single-flight's job and is pinned by + # ``test_only_one_probe_is_ever_in_flight``, not by timing here. + assert counter["rounds"] >= 2 + + +def test_subclass_maxsize_override_does_not_shrink_shared_cache(monkeypatch): + """``_SERVER_TYPE_CACHE_MAXSIZE`` is resolved on ``Authorizer``, so a + subclass cannot evict cached detections belonging to other authorizers.""" + fake_get, _counter = make_probe_counter({PLATFORM_HEALTH}) + monkeypatch.setattr(HTTP_GET, fake_get) + + class SmallCacheAuthorizer(AccessTokenAuthorizer): + _SERVER_TYPE_CACHE_MAXSIZE = 1 + + AccessTokenAuthorizer("tok", "https://keep-a.example.com") + AccessTokenAuthorizer("tok", "https://keep-b.example.com") + SmallCacheAuthorizer("tok", "https://small.example.com") + + cache = Authorizer._server_type_cache + assert "https://keep-a.example.com" in cache + assert "https://keep-b.example.com" in cache + assert "https://small.example.com" in cache + + +def test_detection_flights_are_retired(monkeypatch): + """The in-flight registry holds an entry only while a probe is running, so + it is bounded by live concurrency, not by how many URLs were ever seen.""" + fake_get, _counter = make_probe_counter({PLATFORM_HEALTH}) + monkeypatch.setattr(HTTP_GET, fake_get) + + for i in range(Authorizer._SERVER_TYPE_CACHE_MAXSIZE + 10): + AccessTokenAuthorizer("tok", f"https://flight-{i}.example.com") + + assert Authorizer._server_type_flights == {} + + +def test_detection_flight_is_retired_after_failure(monkeypatch): + """A failed flight must not linger, or the next caller would join a spent + one instead of re-probing.""" + fake_get, _counter = make_probe_counter(set()) + monkeypatch.setattr(HTTP_GET, fake_get) + + with pytest.raises(SecretServerError, match="Unable to detect server type"): + AccessTokenAuthorizer("tok", "https://down.example.com") + + assert Authorizer._server_type_flights == {} + + +def test_clear_cache_clears_detections_and_leaves_no_flights(monkeypatch): + fake_get, _counter = make_probe_counter({PLATFORM_HEALTH}) + monkeypatch.setattr(HTTP_GET, fake_get) + + AccessTokenAuthorizer("tok", "https://platform.example.com") + assert Authorizer._server_type_cache + + Authorizer.clear_server_type_cache() + assert not Authorizer._server_type_cache + assert Authorizer._server_type_flights == {} + + +def test_failure_still_re_probes_under_single_flight(monkeypatch): + """A failed probe is not cached, and the detection lock does not wedge the + next attempt.""" + fake_get, counter = make_probe_counter(set()) # nothing healthy + monkeypatch.setattr(HTTP_GET, fake_get) + base_url = "https://down.example.com" + + for _ in range(2): + with pytest.raises(SecretServerError, match="Unable to detect server type"): + AccessTokenAuthorizer("tok", base_url) + + assert base_url not in Authorizer._server_type_cache + assert counter["rounds"] == 2 + + +def test_only_one_probe_is_ever_in_flight(monkeypatch): + """Directly pin the single-flight property. + + Rather than infer it from a count a fast mock could reach by luck, this + widens the probe window and asserts two are never in flight at once. + """ + import time + + base_url = "https://platform.example.com" + state = {"in_flight": 0, "max_in_flight": 0, "probes": 0} + guard = threading.Lock() + + def fake_get(url, *args, **kwargs): + with guard: + state["in_flight"] += 1 + state["probes"] += 1 + state["max_in_flight"] = max(state["max_in_flight"], state["in_flight"]) + time.sleep(0.01) + with guard: + state["in_flight"] -= 1 + return health_response(url.endswith(PLATFORM_HEALTH)) + + monkeypatch.setattr(HTTP_GET, fake_get) + + errors = [] + start = threading.Event() + + def worker(): + start.wait() + try: + AccessTokenAuthorizer("tok", base_url) + except Exception as exc: # pragma: no cover - failure path + errors.append(exc) + + threads = [threading.Thread(target=worker, daemon=True) for _ in range(20)] + for t in threads: + t.start() + start.set() + join_all(threads) + + assert errors == [] + assert state["max_in_flight"] == 1 + # The winning thread probes secret_server then platform; nobody else probes. + assert state["probes"] == 2 + + +def test_failure_path_shares_one_probe_pair(monkeypatch): + """A cohort hitting an unreachable base_url shares the leader's failure: + one probe pair for everyone, not one per caller. Deterministic by + construction: the probe is held until every thread has registered. + """ + thread_count = 12 + guard = threading.Lock() + registered = {"count": 0} + all_registered = threading.Event() + real_register = Authorizer._start_or_join_detection + + def counting_register(key): + result = real_register(key) + with guard: + registered["count"] += 1 + if registered["count"] == thread_count: + all_registered.set() + return result + + monkeypatch.setattr( + Authorizer, "_start_or_join_detection", staticmethod(counting_register) + ) + + state = {"probes": 0, "in_flight": 0, "max_in_flight": 0, "waited_ok": None} + + def unreachable(url, *args, **kwargs): + with guard: + state["probes"] += 1 + state["in_flight"] += 1 + state["max_in_flight"] = max(state["max_in_flight"], state["in_flight"]) + # Recorded, not asserted: an exception here would be swallowed by the + # probe's own error handling and the test would pass vacuously. + state["waited_ok"] = all_registered.wait(timeout=5) + with guard: + state["in_flight"] -= 1 + raise OSError("unreachable") + + monkeypatch.setattr(HTTP_GET, unreachable) + + failures = [] + start = threading.Event() + + def worker(): + start.wait() + try: + AccessTokenAuthorizer("tok", "https://down.example.com") + except SecretServerError as exc: + failures.append(exc) + + threads = [ + threading.Thread(target=worker, daemon=True) for _ in range(thread_count) + ] + for t in threads: + t.start() + start.set() + join_all(threads) + + assert state["waited_ok"] is True, "not every thread registered on the flight" + # Every caller learns that detection failed ... + assert len(failures) == thread_count + # ... from one shared probe pair, not one pair each, and never a burst. + assert state["probes"] == 2 + assert state["max_in_flight"] == 1 + # Each caller gets its own exception carrying the leader's message and + # chained to the leader's -- never the leader's instance itself, whose + # traceback would otherwise be rewritten by every thread re-raising it. + assert len({id(exc) for exc in failures}) == thread_count + assert len({exc.message for exc in failures}) == 1 + assert sum(1 for exc in failures if exc.__cause__ is not None) == thread_count - 1 + + +def test_health_body_error_falls_through_to_next_endpoint(monkeypatch): + """A body that raises something other than ValueError means "unhealthy, + try the next endpoint", never "abort detection".""" + + def fake_get(url, *args, **kwargs): + if url.endswith(SECRET_SERVER_HEALTH): + return HostileBody() + return health_response(True) + + monkeypatch.setattr(HTTP_GET, fake_get) + + authorizer = AccessTokenAuthorizer("tok", "https://platform.example.com") + assert authorizer._server_type == "platform" + + +def test_leader_interrupt_is_not_broadcast_to_waiters(monkeypatch): + """A KeyboardInterrupt in the leader belongs to the leader. Waiters get an + ordinary SecretServerError their handlers can catch, not a foreign + interrupt raised in the middle of their own work.""" + thread_count = 6 + guard = threading.Lock() + registered = {"count": 0} + all_registered = threading.Event() + real_register = Authorizer._start_or_join_detection + + def counting_register(key): + result = real_register(key) + with guard: + registered["count"] += 1 + if registered["count"] == thread_count: + all_registered.set() + return result + + monkeypatch.setattr( + Authorizer, "_start_or_join_detection", staticmethod(counting_register) + ) + + def interrupted_probe(url, *args, **kwargs): + all_registered.wait(timeout=5) + raise KeyboardInterrupt() + + monkeypatch.setattr(HTTP_GET, interrupted_probe) + + outcomes = [] + start = threading.Event() + + def worker(): + start.wait() + try: + AccessTokenAuthorizer("tok", "https://down.example.com") + except BaseException as exc: # the interrupt is the point of the test + with guard: + outcomes.append(exc) + + threads = [ + threading.Thread(target=worker, daemon=True) for _ in range(thread_count) + ] + for t in threads: + t.start() + start.set() + join_all(threads) + + interrupts = [e for e in outcomes if isinstance(e, KeyboardInterrupt)] + errors = [e for e in outcomes if isinstance(e, SecretServerError)] + assert len(interrupts) == 1 # the leader, and only the leader + assert len(errors) == thread_count - 1 + assert all("interrupted" in e.message for e in errors) + assert Authorizer._server_type_flights == {} + + +def test_waiters_take_over_from_a_stalled_leader(monkeypatch): + """A leader that outruns every bound a probe can have no longer strands the + callers waiting on it: they time out, retire its flight and probe.""" + import time + + # 1 s: long enough that the taking-over waiter's two instant probes + # cannot be pre-empted by a second timeout on a slow runner, short + # enough to stay well inside the 5 s waiter join bound below. + monkeypatch.setattr("delinea.secrets.server._DETECTION_WAIT_TIMEOUT", 1.0) + key = "https://platform.example.com" + release_leader = threading.Event() + calls = {"n": 0} + guard = threading.Lock() + + def fake_get(url, *args, **kwargs): + with guard: + calls["n"] += 1 + # Hang by thread identity, not by call ordinal: if the leader were + # descheduled between registering its flight and probing, a waiter + # could otherwise be the one that gets stuck. + if threading.current_thread().name == "leader": + # Longer than the waiters' join bound below, so the waiters can + # only finish by taking over. + release_leader.wait(timeout=30) + return health_response(url.endswith(PLATFORM_HEALTH)) + + monkeypatch.setattr(HTTP_GET, fake_get) + + results = {} + + def worker(name): + def run(): + results[name] = AccessTokenAuthorizer("tok", key)._server_type + + return run + + leader = threading.Thread(target=worker("leader"), name="leader", daemon=True) + leader.start() + waiters = [threading.Thread(target=worker(f"w{i}"), daemon=True) for i in range(3)] + try: + deadline = time.monotonic() + 5 + while ( + key not in Authorizer._server_type_flights and time.monotonic() < deadline + ): + time.sleep(0.005) + assert key in Authorizer._server_type_flights, "leader never registered" + for t in waiters: + t.start() + join_all(waiters, timeout=5) + assert not release_leader.is_set() + assert all(results[f"w{i}"] == "platform" for i in range(3)) + # The leader's hung probe plus exactly one probe pair from the single + # waiter that took over; the other two joined its flight. + assert calls["n"] == 3 + finally: + # Always let the leader go AND wait for it, so a failure here cannot + # leak a thread that keeps probing (and writing the cache) into the + # tests that run next. Once released it finishes within milliseconds. + release_leader.set() + join_all([leader]) + assert results["leader"] == "platform" + assert Authorizer._server_type_flights == {} + + +def test_leader_sees_the_same_error_type_as_its_waiters(monkeypatch): + """A probe failure that is not a SecretServerError reaches every caller + as one: waiters via ``_shared_failure``, and the leader too, so the type a + caller must catch does not depend on which thread won the registration.""" + + def exploding_probe(self, base_url): + raise RuntimeError("probe exploded") + + monkeypatch.setattr(Authorizer, "_probe_server_type", exploding_probe) + with pytest.raises(SecretServerError) as excinfo: + AccessTokenAuthorizer("tok", "https://x.example.com") + assert isinstance(excinfo.value.__cause__, RuntimeError) + assert "RuntimeError" in excinfo.value.message + assert Authorizer._server_type_flights == {} + assert "https://x.example.com" not in Authorizer._server_type_cache + + +def test_clear_cache_drops_a_stranded_flight(): + key = "https://stranded.example.com" + _cached, _flight, is_leader = Authorizer._start_or_join_detection(key) + assert is_leader and key in Authorizer._server_type_flights + + Authorizer.clear_server_type_cache() + assert Authorizer._server_type_flights == {} + + +# --------------------------------------------------------------------------- +# Round 9: stale leaders, subclass errors, the waiter bound +# --------------------------------------------------------------------------- + + +def test_stale_leader_does_not_overwrite_a_cleared_cache(monkeypatch): + """A probe that began before ``clear_server_type_cache`` must not write its + answer back afterwards; only the flight still registered may cache.""" + import time + + key = "https://switched.example.com" + release_leader = threading.Event() + + def fake_get(url, *args, **kwargs): + if threading.current_thread().name == "leader": + release_leader.wait(timeout=10) + return health_response(url.endswith(SECRET_SERVER_HEALTH)) # old answer + return health_response(url.endswith(PLATFORM_HEALTH)) # current answer + + monkeypatch.setattr(HTTP_GET, fake_get) + results = {} + leader = threading.Thread( + target=lambda: results.update( + leader=AccessTokenAuthorizer("tok", key)._server_type + ), + name="leader", + daemon=True, + ) + leader.start() + try: + deadline = time.monotonic() + 5 + while ( + key not in Authorizer._server_type_flights and time.monotonic() < deadline + ): + time.sleep(0.005) + assert key in Authorizer._server_type_flights, "leader never registered" + Authorizer.clear_server_type_cache() # re-provisioned: forget everything + assert AccessTokenAuthorizer("tok", key)._server_type == "platform" + assert Authorizer._server_type_cache[key] == "platform" + finally: + release_leader.set() + join_all([leader]) + assert results["leader"] == "secret_server" # what it observed, for itself + assert Authorizer._server_type_cache[key] == "platform" # not overwritten + assert Authorizer._server_type_flights == {} + + +def test_shared_failure_tolerates_a_subclass_with_its_own_constructor(): + """A probe override may raise a SecretServerError subclass whose __init__ + takes only a message; waiters must still get a shareable error.""" + + class MessageOnly(SecretServerError): + def __init__(self, message): + super().__init__(message) + + shared = Authorizer._shared_failure(MessageOnly("probe said no")) + assert isinstance(shared, SecretServerError) + assert shared.message == "probe said no" + + +def test_waiter_bound_covers_connect_and_read_for_both_probes(): + """``requests`` applies its timeout per socket operation, so a live leader + can spend two timeouts per probe; the waiter bound must allow for four.""" + assert _DETECTION_WAIT_TIMEOUT == 4 * DEFAULT_REQUEST_TIMEOUT + 5 + + +def test_waiter_on_a_superseded_flight_takes_the_current_answer(monkeypatch): + """A waiter whose leader was retired by a clear, and then failed, must not + raise that stale failure while the newer detection's answer is cached.""" + import time + + key = "https://superseded.example.com" + release_leader = threading.Event() + joined = threading.Event() + + def fake_get(url, *args, **kwargs): + if threading.current_thread().name == "leader": + release_leader.wait(timeout=10) + return health_response(False) # the stale leader fails outright + return health_response(url.endswith(PLATFORM_HEALTH)) + + monkeypatch.setattr(HTTP_GET, fake_get) + real_start = Authorizer._start_or_join_detection + + def recording_start(k): + outcome = real_start(k) + if threading.current_thread().name == "waiter" and outcome[1] is not None: + joined.set() # the waiter is now parked on the leader's flight + return outcome + + monkeypatch.setattr( + Authorizer, "_start_or_join_detection", staticmethod(recording_start) + ) + results = {} + + def detect(name): + try: + results[name] = AccessTokenAuthorizer("tok", key)._server_type + except SecretServerError as exc: + results[name] = exc + + leader = threading.Thread( + target=detect, args=("leader",), name="leader", daemon=True + ) + waiter = threading.Thread( + target=detect, args=("waiter",), name="waiter", daemon=True + ) + leader.start() + try: + deadline = time.monotonic() + 5 + while ( + key not in Authorizer._server_type_flights and time.monotonic() < deadline + ): + time.sleep(0.005) + assert key in Authorizer._server_type_flights, "leader never registered" + waiter.start() + assert joined.wait(timeout=5), "waiter never joined the leader's flight" + Authorizer.clear_server_type_cache() # retires the leader's flight + assert AccessTokenAuthorizer("tok", key)._server_type == "platform" + finally: + release_leader.set() + # Join only what was started: a failure before ``waiter.start()`` must + # report itself, not a RuntimeError from joining an unstarted thread. + join_all([t for t in (leader, waiter) if t.ident is not None]) + assert isinstance(results["leader"], SecretServerError) # its own observation + assert results["waiter"] == "platform" # not the stale failure diff --git a/tox.ini b/tox.ini index 2420de2..53adb3e 100644 --- a/tox.ini +++ b/tox.ini @@ -6,15 +6,19 @@ # Docs for tox config -> https://tox.readthedocs.io/en/latest/config.html [tox] -envlist = 3.8, 3.9, 3.10, 3.11, 3.12 +# Python 3.8/3.9 dropped: fixed requests/urllib3 pins require Python >= 3.10 (work item 741117) +envlist = 3.10, 3.11, 3.12 isolated_build = True skipsdist = True [testenv] +# requirements-test.txt inherits requirements.txt (runtime pins) and adds only +# pytest + python-dotenv, so tests exercise the same requests/urllib3/idna +# versions consumers get, not floating "latest" package names -- and without +# installing the build/lint toolchain (tox, flit, black) into every test +# virtualenv, which added install time to each matrix job for no coverage. deps = - pytest - requests - python-dotenv + -r requirements-test.txt passenv = TSS_USERNAME TSS_PASSWORD