From d549c0f18eef01a8da2ac934e5bebc0d85cf0552 Mon Sep 17 00:00:00 2001 From: Leo Parente <23251360+leoparente@users.noreply.github.com> Date: Tue, 21 Jul 2026 13:41:37 -0300 Subject: [PATCH] fix(auth): keep token endpoint on target scheme for insecure targets The auth token URL scheme was derived from tls_verify plus DIODE_SKIP_TLS_VERIFY, which forced HTTPS for an insecure grpc:// (or http://) target whenever DIODE_SKIP_TLS_VERIFY was set, producing an SSL WRONG_VERSION_NUMBER against a plain-HTTP server. Track the target's secure/insecure scheme separately and build the auth URL from it. DIODE_SKIP_TLS_VERIFY now only controls certificate verification, never http vs https. Fixes #101 Co-Authored-By: Claude Opus 4.8 --- netboxlabs/diode/sdk/client.py | 31 ++++++++------- tests/test_client.py | 70 ++++++++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+), 13 deletions(-) diff --git a/netboxlabs/diode/sdk/client.py b/netboxlabs/diode/sdk/client.py index b57101d..3645769 100644 --- a/netboxlabs/diode/sdk/client.py +++ b/netboxlabs/diode/sdk/client.py @@ -347,6 +347,12 @@ def __init__( _DIODE_CERT_FILE_ENVVAR_NAME, cert_file ) self._target, self._path, self._tls_verify = parse_target(target) + # Whether the target scheme is secure (grpcs/https). Kept separately from + # tls_verify, which only controls certificate verification: tls_verify is + # False both for an insecure grpc:// target and for a grpcs:// target with + # verification disabled, so it cannot by itself tell the auth endpoint + # which scheme to use. + self._secure = urlparse(target).scheme in ("grpcs", "https") # Load certificates once if needed self._certificates = ( @@ -547,6 +553,7 @@ def _authenticate(self, scope: str): self._certificates, self._cert_file, max_retries=self._max_auth_retries, + secure=self._secure, ) access_token = authentication_client.authenticate() self._metadata = list( @@ -942,9 +949,11 @@ def __init__( initial_retry_delay: float | None = None, max_retry_delay: float | None = None, sleep: Callable[[float], None] | None = None, + secure: bool = True, ): self._target = target self._tls_verify = tls_verify + self._secure = secure self._client_id = client_id self._client_secret = client_secret self._path = path @@ -1059,19 +1068,15 @@ def _get_auth_url(self) -> str: return f"{path}/auth/token" def _get_full_auth_url(self) -> str: - """Construct full authentication URL with scheme and authority.""" - # Determine the correct scheme - # If tls_verify is False, check if SKIP_TLS_VERIFY was set - # If it was set, the original scheme was likely HTTPS but verification is disabled - skip_tls_env = os.getenv(_DIODE_SKIP_TLS_VERIFY_ENVVAR_NAME, "").lower() - skip_tls_from_env = skip_tls_env in ["true", "1", "yes", "on"] - - # Use HTTPS if: - # 1. tls_verify is True, OR - # 2. tls_verify is False but SKIP_TLS_VERIFY is set (original was HTTPS) - use_https = self._tls_verify or (not self._tls_verify and skip_tls_from_env) - scheme = "https" if use_https else "http" - + """ + Construct full authentication URL, matching the target's scheme. + + The scheme follows the target (https for grpcs/https, http for grpc/http) + and is independent of certificate verification, which is handled separately + via the session's verify setting. This keeps an insecure grpc:// target on + HTTP even when DIODE_SKIP_TLS_VERIFY is set. + """ + scheme = "https" if self._secure else "http" path = self._path.rstrip("/") if self._path else "" return f"{scheme}://{self._target}{path}/auth/token" diff --git a/tests/test_client.py b/tests/test_client.py index 286f044..a89457f 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -665,6 +665,7 @@ def test_diode_authentication_url_with_path(mock_diode_authentication, path): target="localhost:8081", path=path, tls_verify=False, + secure=False, client_id="test_client_id", client_secret="test_client_secret", scope="diode:ingest", @@ -690,6 +691,75 @@ def test_diode_authentication_url_with_path(mock_diode_authentication, path): assert url == expected_url +@pytest.mark.parametrize( + ("secure", "tls_verify", "skip_tls_env", "expected_scheme"), + [ + # (scheme secure?, verify certs?, DIODE_SKIP_TLS_VERIFY, expected auth scheme) + (False, False, None, "http"), # grpc:// + # grpc:// + skip must stay HTTP (the #101 fix), not flip to HTTPS. + (False, False, "true", "http"), + (True, True, None, "https"), # grpcs:// + # grpcs:// + skip is preserved: HTTPS with cert verification disabled. + (True, False, "true", "https"), + # Independence guards: the scheme follows `secure`, never `tls_verify`. + # A scheme = self._tls_verify regression would fail both of these. + (True, False, None, "https"), + (False, True, None, "http"), + ], +) +def test_auth_url_scheme_follows_target( + mock_diode_authentication, monkeypatch, secure, tls_verify, skip_tls_env, expected_scheme +): + """Auth endpoint scheme follows the target scheme, independent of tls_verify/env.""" + if skip_tls_env is None: + monkeypatch.delenv("DIODE_SKIP_TLS_VERIFY", raising=False) + else: + monkeypatch.setenv("DIODE_SKIP_TLS_VERIFY", skip_tls_env) + + auth = _DiodeAuthentication( + target="host:8080", + path="", + tls_verify=tls_verify, + client_id="test_client_id", + client_secret="test_client_secret", + scope="diode:ingest", + sdk_name="diode-sdk-python", + sdk_version="0.1.0", + app_name="test-app", + app_version="1.0.0", + secure=secure, + ) + + assert auth._get_full_auth_url() == f"{expected_scheme}://host:8080/auth/token" + + +@pytest.mark.parametrize( + ("target", "expected_secure"), + [ + ("grpc://localhost:8081", False), + ("http://localhost:8081", False), + ("grpcs://localhost:8081", True), + ("https://localhost:8081", True), + ], +) +def test_client_secure_flag_follows_target_scheme( + mock_diode_authentication, monkeypatch, target, expected_secure +): + """DiodeClient._secure reflects the target scheme even with DIODE_SKIP_TLS_VERIFY set.""" + monkeypatch.setenv("DIODE_SKIP_TLS_VERIFY", "true") + client = DiodeClient( + target=target, + app_name="my-producer", + app_version="0.0.1", + client_id="abcde", + client_secret="123456", + ) + assert client._secure is expected_secure + # tls_verify is driven off skip-verify and is False here regardless of scheme, + # which is exactly why it cannot be reused to pick the auth scheme. + assert client.tls_verify is False + + def test_diode_authentication_request_exception(mock_diode_authentication): """Test that an exception during the request raises a DiodeConfigError.""" auth = _DiodeAuthentication(