feat: gRPC/HTTP collector TLS and mTLS - #419
Conversation
Add SW_AGENT_FORCE_TLS and SSL CA/cert/key paths; share tls helpers across gRPC and HTTP; convert PKCS#1 keys to PKCS#8 for HTTP stacks that reject PKCS#1. Follow symlinks so Kubernetes secret mounts work. Never raise TLS misconfig into the host process: warn and degrade (plaintext / system trust / one-way TLS), including OSError from SSLContext load races and path expanduser/resolve failures. Validate CA and client PEMs when building material so bad content does not defer failure to connect time; FORCE_TLS fallback never attaches client certs, and credential build failure may warn and stay plaintext rather than abort start. Keep HTTP mTLS temp PEMs fork-safe via register_at_fork rebind. Drop test-only ssl_target_name_override; TLS peer-name follows grpc.default_authority. Generate e2e PEMs via shared gen-e2e-tls-certs.sh in digest-pinned alpine/openssl (no apk; PEMs not committed). mTLS e2e healthchecks the sharing-server port.
wu-sheng
left a comment
wu-sheng
left a comment
There was a problem hiding this comment.
Reviewed commit a4dfb421ee20d8f0a02b452910f51f1d7d12da95. I found three reproducible issues in the new TLS helper: the synchronous HTTP CA path becomes stale after secret rotation, PKCS#1 normalization corrupts keys with a valid preamble, and async HTTP rejects CA files that its own validation accepts. The inline comments include explanations, standalone reproduction commands, observed/expected behavior, and suggested fixes.
Validation performed:
- 96 focused unit tests passed:
test_tls.py,test_grpc_channel.py,test_grpc_ready_gate.py, andtest_shutdown_queue.py. - Live checks passed through the sync/aio gRPC factories and sync/aio HTTP clients using the PR's generated e2e certificates. Valid TLS/mTLS succeeded; unrelated server CAs, hostname mismatches, missing client certificates, and untrusted client certificates were rejected. Separate gRPC checks also covered single and multiple backends.
- The handshake checks used local Python gRPC and HTTPS test servers. For mTLS, the gRPC server used
require_client_auth=Trueand the HTTPS server usedssl.CERT_REQUIRED, both with the intended client CA. These servers performed the client-certificate verification. These were supplemental review checks, not runs against OAP or an external third-party service. - I did not run the real-OAP e2e suite. This PR's gRPC mTLS Compose case connects directly to OAP's receiver-sharing-server on port 11811; its HTTP mTLS scenario instead requires an appropriate TLS terminator.
The successful baseline/negative handshake checks do not cover the file-handling edge cases reported inline.
Extract PKCS#1 / CERTIFICATE PEM between BEGIN/END only so preamble and UTF-8 BOM cannot break b64decode or aio SSLContext cadata. Always verify HTTP with a process-lifetime CA temp snapshot (not the resolved K8s ..data path) so secret rotation cannot invalidate an open session.
|
Thanks for the detailed P2 reports and repros. Addressed in P2-1 — HTTP CA path after Kubernetes secret rotation P2-2 — PKCS#1 preamble outside PEM delimiters P2-3 — aio SSLContext rejecting CA that Covered by unit tests aligned with your repros ( |
wu-sheng
left a comment
wu-sheng
left a comment
There was a problem hiding this comment.
Rechecked 3d3d5443f2d70d62b08a5672882b0d95fff4f426.
The three original findings are fixed. I reran the posted PEM/key reproductions and verified the fixes against a local HTTPS server requiring client certificates: the same synchronous session still returned HTTP 200 after Kubernetes-style CA symlink rotation, and both sync/async HTTP succeeded with BOM/UTF-8 CA preambles and a PKCS#1 key containing a preamble.
The follow-up introduces two new synchronous HTTP regressions, detailed inline with runnable reproductions and suggested fixes: loss of the configured CA when temporary storage is unavailable, and rejection of OpenSSL trusted-certificate PEMs.
Validation: 99 focused unit tests passed. All 88 GitHub checks were successful when inspected, including the real-OAP gRPC TLS/mTLS cases across Python 3.10–3.14. My own handshake checks used local Python HTTPS servers; I did not run the real-OAP e2e suite locally.
| if root_certificates is not None: | ||
| try: | ||
| verify: object = _ca_verify_temp_file(root_certificates) | ||
| except OSError as exc: |
There was a problem hiding this comment.
[P2] Preserve the readable custom CA when its snapshot cannot be written
The snapshot change introduces a writable-temp-directory requirement even for one-way HTTP TLS. If creating the snapshot raises OSError, this branch replaces the successfully loaded private CA with verify=True (Requests' default trust roots). A collector signed by that private CA then fails verification, despite the configured CA file still being readable. Before this update, the synchronous reporter used that readable file directly and needed no temporary writes. This affects, for example, a container with a read-only filesystem and no writable temporary mount.
I reproduced the regression against a local HTTPS server using a genuinely non-writable directory, without mocking file creation: the previous helper returned the original CA path and the request succeeded with HTTP 200; the updated helper logged PermissionError, returned verify=True, and the request failed with CERTIFICATE_VERIFY_FAILED.
Please preserve the loaded custom trust when snapshot creation fails, either through an in-memory context or a fallback to a still-readable configured CA path. The fallback should not discard a usable CA merely because a copy cannot be written.
Minimal reproduction of the settings error, run from the PR checkout with dependencies installed, as a non-root user on Linux/macOS. It uses this PR's existing PEM fixture and needs no collector:
poetry run python - <<'PY'
import os
import ssl
import tempfile
from pathlib import Path
from skywalking import config
from skywalking.utils import tls
from tests.unit.test_tls import _TEST_CA_CERT
config.agent_force_tls = False
config.agent_ssl_cert_chain_path = ''
config.agent_ssl_key_path = ''
with tempfile.TemporaryDirectory() as tmp:
ca = Path(tmp) / 'ca.crt'
ca.write_bytes(_TEST_CA_CERT)
config.agent_ssl_trusted_ca_path = str(ca)
ssl.create_default_context(cafile=str(ca))
blocked = Path(tmp) / 'readonly'
blocked.mkdir()
blocked.chmod(0o555)
saved_tempdir = tempfile.tempdir
try:
if os.access(blocked, os.W_OK):
raise SystemExit('Run as an unprivileged user on a POSIX filesystem.')
tempfile.tempdir = str(blocked)
print('Configured CA is readable:', ca.read_bytes() == _TEST_CA_CERT)
print('Custom CA material loaded:', tls.tls_pem_material()[0] is not None)
verify, cert = tls.requests_tls_settings()
print('Requests verify:', verify)
print('Client certificate:', cert)
finally:
tempfile.tempdir = saved_tempdir
blocked.chmod(0o755)
PYObserved output, plus the warning that the CA snapshot could not be written:
Configured CA is readable: True
Custom CA material loaded: True
Requests verify: True
Client certificate: None
Expected: Requests continues to use the readable private CA for one-way TLS. Running the equivalent setup on a4dfb42 returns the CA file path instead of True.
There was a problem hiding this comment.
Fixed in 1327f8b: fall back to the configured CA path when the snapshot cannot be written; covered by test_requests_ca_temp_failure_falls_back_to_configured_path.
| except OSError as exc: | ||
| # ssl.SSLError subclasses OSError on CPython. | ||
| raise ValueError(f'Invalid trusted CA PEM {path}: {exc}') from exc | ||
| return _extract_pem_blocks(data, 'CERTIFICATE') |
There was a problem hiding this comment.
[P2] Preserve OpenSSL trusted-certificate blocks for synchronous HTTP
The new extraction only retains CERTIFICATE blocks, but OpenSSL also accepts TRUSTED CERTIFICATE PEMs produced by openssl x509 -trustout. A file containing only trusted-certificate blocks passes the cafile validation immediately above, then raises No CERTIFICATE PEM block found here. With agent_force_tls=False, the agent selects http://; with agent_force_tls=True, it discards the configured CA and falls back to default trust. Mixed bundles silently lose their trusted-certificate entries.
This is a new regression for the synchronous HTTP reporter, which previously passed the OpenSSL-accepted CA file directly to Requests. The previous async cadata path already had a limitation with this format, so this finding is specifically about losing working synchronous HTTP support.
Please retain supported trusted-certificate blocks and their trust attributes in the Requests CA snapshot. Merely relabeling them as CERTIFICATE can discard trust restrictions; normalization should not treat valid trust-store entries as preamble.
Reproduction from the PR checkout, with project dependencies and OpenSSL installed:
poetry run python - <<'PY'
import ssl
import subprocess
import tempfile
from pathlib import Path
from skywalking import config
from skywalking.utils import tls
from tests.unit.test_tls import _TEST_CA_CERT
config.agent_force_tls = False
config.agent_ssl_cert_chain_path = ''
config.agent_ssl_key_path = ''
with tempfile.TemporaryDirectory() as tmp:
ca = Path(tmp) / 'ca.pem'
trusted = Path(tmp) / 'trusted.pem'
ca.write_bytes(_TEST_CA_CERT)
subprocess.run([
'openssl', 'x509', '-in', str(ca), '-addtrust', 'serverAuth',
'-trustout', '-out', str(trusted),
], check=True)
ssl.create_default_context(cafile=str(trusted))
print('OpenSSL cafile accepted:', trusted.read_text().splitlines()[0])
config.agent_ssl_trusted_ca_path = str(trusted)
print('Agent TLS material:', tls.tls_pem_material())
print('Agent HTTP scheme:', tls.collector_http_scheme())
PYObserved output, plus the warning about No CERTIFICATE PEM block found:
OpenSSL cafile accepted: -----BEGIN TRUSTED CERTIFICATE-----
Agent TLS material: None
Agent HTTP scheme: http://
Expected: the configured, accepted trusted CA remains usable for synchronous HTTPS. On a4dfb42, the same configuration returns TLS material, selects https://, and uses the trusted CA file as Requests' verify path.
There was a problem hiding this comment.
Fixed in 1327f8b: retain TRUSTED CERTIFICATE blocks and trust attributes in the CA snapshot; covered by test_trusted_certificate_pem_enables_sync_http_tls.
When the HTTP CA snapshot cannot be written, fall back to the still-readable configured CA path instead of Requests' system trust store. Extract both CERTIFICATE and TRUSTED CERTIFICATE blocks (labels/trust attrs preserved) so openssl -trustout CAs remain usable for sync HTTPS; aio falls back to cafile when cadata rejects TRUSTED CERTIFICATE.
|
Thanks for the follow-up review. The two new findings are addressed in P2 — Preserve the readable custom CA when its snapshot cannot be written P2 — Preserve OpenSSL trusted-certificate blocks for synchronous HTTP Covered by |
wu-sheng
left a comment
wu-sheng
left a comment
There was a problem hiding this comment.
Rechecked current head 1327f8b2b2a6da6f7c52dd44fbadd3ef5413ef8c.
The latest fixes pass their standalone reproductions: a readable CA remains usable when snapshot creation fails, and a pure TRUSTED CERTIFICATE file remains usable. The earlier PEM-preamble and normal CA-snapshot rotation fixes also passed the local mTLS checks.
Two combined cases remain, detailed inline with runnable reproductions and suggested fixes:
- Synchronous HTTP: no writable temporary storage together with Kubernetes-style CA rotation.
- Asynchronous HTTP: a CA bundle containing both trusted-certificate and ordinary certificate blocks.
Validation from this review: 102 focused unit tests passed. Local HTTPS and in-memory TLS checks reproduced the two remaining issues. All 88 GitHub checks are successful, including the real-OAP gRPC TLS/mTLS cases across Python 3.10–3.14. I did not run the real-OAP e2e suite locally.
| 'falling back to configured CA path for HTTP verify.', | ||
| exc, | ||
| ) | ||
| verify = str(ca_path) |
There was a problem hiding this comment.
[P2] Preserve the configured symlink in the no-snapshot fallback
ssl_file_path() resolves symlinks, so this fallback stores the versioned v1/ca.crt target instead of the configured ca.crt -> ..data/ca.crt path. When temporary storage is unavailable, a subsequent Kubernetes secret update still invalidates the existing Requests session: replacing ..data and deleting v1 leaves the configured CA readable but makes session.verify point to a deleted file. The no-temp and rotation cases need to work together.
I verified this with a local HTTPS server and an actual non-writable temp directory, without mocking file creation: the session returned HTTP 200 before rotation, then raised the invalid-CA-path OSError afterward. Assigning the unresolved configured symlink to session.verify restored HTTP 200.
Please validate the configured CA but retain its absolute, expanded path without resolving its symlinks in this fallback. The normal writable-temp snapshot path can remain unchanged.
Minimal reproduction from the PR checkout with dependencies installed, as a non-root user on Linux/macOS. No collector is required: Requests rejects the missing CA path before connecting.
poetry run python - <<'PY'
import os
import shutil
import tempfile
from pathlib import Path
import requests
from skywalking import config
from skywalking.utils.tls import configure_requests_session
from tests.unit.test_tls import _TEST_CA_CERT
config.agent_force_tls = False
config.agent_ssl_cert_chain_path = ''
config.agent_ssl_key_path = ''
with tempfile.TemporaryDirectory() as tmp, requests.Session() as session:
root = Path(tmp)
for version in ('v1', 'v2'):
(root / version).mkdir()
(root / version / 'ca.crt').write_bytes(_TEST_CA_CERT)
(root / '..data').symlink_to('v1')
ca = root / 'ca.crt'
ca.symlink_to('..data/ca.crt')
config.agent_ssl_trusted_ca_path = str(ca)
blocked = root / 'readonly'
blocked.mkdir()
blocked.chmod(0o555)
saved_tempdir = tempfile.tempdir
try:
if os.access(blocked, os.W_OK):
raise SystemExit('Run as a non-root user on a POSIX filesystem.')
tempfile.tempdir = str(blocked)
session.trust_env = False
configure_requests_session(session)
print('Fallback uses v1:', Path(session.verify).parent.name == 'v1')
(root / '..data-next').symlink_to('v2')
os.replace(root / '..data-next', root / '..data')
shutil.rmtree(root / 'v1')
print('Configured CA remains valid:', ca.read_bytes() == _TEST_CA_CERT)
print('Stored verify path exists:', Path(session.verify).exists())
try:
session.post('https://127.0.0.1:1/v3/segment', json={}, timeout=1)
except OSError as exc:
print(type(exc).__name__ + ':', exc)
finally:
tempfile.tempdir = saved_tempdir
blocked.chmod(0o755)
PYObserved output, plus the expected warning that snapshot creation failed:
Fallback uses v1: True
Configured CA remains valid: True
Stored verify path exists: False
OSError: Could not find a suitable TLS CA certificate bundle, invalid path: .../v1/ca.crt
Expected: after rotation, the fallback still points through the readable configured CA symlink and does not fail because the old version directory was removed.
| ctx = ssl.create_default_context( | ||
| cadata=root_certificates.decode('ascii'), | ||
| ) | ||
| except (OSError, ValueError) as exc: |
There was a problem hiding this comment.
[P2] Load mixed trusted-certificate bundles through cafile
For a bundle containing both TRUSTED CERTIFICATE and ordinary CERTIFICATE blocks, Python's cadata loader can successfully load the ordinary blocks while silently skipping the trusted ones. No exception is raised, so the new cafile fallback is never used. The asynchronous HTTP reporter then cannot verify a collector whose root is in a trusted-certificate block, even though direct cafile loading accepts the complete bundle. Skipping trusted blocks can also discard their trust restrictions.
Please select the cafile loader whenever the bundle contains a trusted-certificate block, using the snapshot/configured-path fallback as appropriate. Success from cadata does not establish that every supported CA block was loaded.
In an in-memory TLS handshake, direct cafile loading retained both entries and completed the handshake; ssl_context_for_collector() retained only the unrelated ordinary certificate and failed certificate verification.
The following reproduction checks both the trust-store entries and the handshake. Run it from the PR checkout with project dependencies and OpenSSL installed (verified with Python 3.10). It uses existing test fixtures and requires no running server or open port. The fixture with an available private key serves as the in-memory TLS server identity; this checks client-side verification of the server, without OAP or mTLS client authentication:
poetry run python - <<'PY'
"""Reproduce mixed CA loading with an in-memory TLS handshake."""
import ssl
import subprocess
import tempfile
from pathlib import Path
from skywalking import config
from skywalking.utils.tls import ssl_context_for_collector
from tests.unit.test_tls import _TEST_CA_CERT, _TEST_CLIENT_CERT, _TEST_CLIENT_KEY_PKCS1
def handshake(client_context, cert, key):
server_context = ssl.SSLContext(ssl.PROTOCOL_TLS_SERVER)
server_context.load_cert_chain(str(cert), str(key))
ci, co, si, so = (ssl.MemoryBIO() for _ in range(4))
client = client_context.wrap_bio(ci, co, server_hostname='skywalking-test-client')
server = server_context.wrap_bio(si, so, server_side=True)
done = [False, False]
for _ in range(20):
for i, (peer, out, target) in enumerate(((client, co, si), (server, so, ci))):
if not done[i]:
try:
peer.do_handshake()
done[i] = True
except ssl.SSLWantReadError:
pass
data = out.read()
if data:
target.write(data)
if all(done):
return 'OK'
raise RuntimeError('handshake did not finish')
with tempfile.TemporaryDirectory() as directory:
root = Path(directory)
ca, cert, key, trusted, bundle = (root / name for name in
('ca.pem', 'server.pem', 'server.key', 'trusted.pem', 'bundle.pem'))
ca.write_bytes(_TEST_CA_CERT)
cert.write_bytes(_TEST_CLIENT_CERT)
key.write_bytes(_TEST_CLIENT_KEY_PKCS1)
# Both fixtures are self-signed. Export the one with an available private key
# as TRUSTED CERTIFICATE and use it as the local TLS server's identity.
subprocess.run(['openssl', 'x509', '-in', str(cert), '-addtrust', 'serverAuth',
'-trustout', '-out', str(trusted)], check=True)
bundle.write_bytes(trusted.read_bytes() + ca.read_bytes())
config.agent_force_tls = False
config.agent_ssl_trusted_ca_path = str(bundle)
config.agent_ssl_cert_chain_path = ''
config.agent_ssl_key_path = ''
contexts = [('direct cafile', ssl.create_default_context(cafile=str(bundle))),
('agent helper', ssl_context_for_collector())]
for label, context in contexts:
print(f'{label}: x509={context.cert_store_stats()["x509"]}')
try:
print(f'{label}: handshake={handshake(context, cert, key)}')
except ssl.SSLCertVerificationError as exc:
print(f'{label}: handshake=FAIL ({exc.verify_message})')
PYObserved output:
direct cafile: x509=2
direct cafile: handshake=OK
agent helper: x509=1
agent helper: handshake=FAIL (self-signed certificate)
Expected: the agent context retains both certificates, matching direct cafile loading. The existing pure-TRUSTED CERTIFICATE case now works; please also cover this mixed-bundle case.
When the HTTP CA snapshot cannot be written, fall back to the configured path without resolving symlinks so K8s secret rotation cannot invalidate session.verify. Prefer cafile whenever the CA PEM includes TRUSTED CERTIFICATE so aio cadata cannot silently drop trusted blocks from a mixed bundle.
Share one PEM material load for HTTP scheme and session/context, prefer configured CA on requests TLS fallback, drop orphaned cert temps and unused _extract_pem_blocks. CA snapshot hot-reload left intentional.
|
Thanks for the review — both P2s are valid on the previous PR head ( They are fixed in
Separately, while re-checking we also found and fixed a few non-blocking polish items (
Pushed in |
2 participants
Summary
Adds collector TLS / mTLS for the Python agent's gRPC and HTTP reporters
via shared helpers in
skywalking/utils/tls.py.SW_AGENT_FORCE_TLSand/or a readable + parseable CA PEM(
SW_AGENT_SSL_TRUSTED_CA_PATH).(
SW_AGENT_SSL_CERT_CHAIN_PATH/SW_AGENT_SSL_KEY_PATH).fork-safe (
os.register_at_fork).grpc.ssl_target_name_override. Withgrpc.default_authorityset to the first configured endpoint, TLS peer-namechecks follow that authority (not the shuffled multi-address dial target).
Kafka protocol is out of scope for these options.
Security / hardening fixes (review follow-ups)
These close concrete gaps against the documented contract
(“TLS misconfiguration must not abort host process start” / correct degrade table):
Path resolution must not block HTTP agent start
ssl_file_path()now wrapsPath.expanduser()+resolve(strict=True)andcatches
OSError/ValueError/RuntimeError(Windows permission errors,symlink loops, unresolvable
~user, etc.).collector_http_scheme()(used from HTTP sync/aio reporter__init__) alsohas a top-level fallback so scheme selection cannot raise into bootstrap.
CA existence vs load TOCTOU (incl. K8s secret rotation)
TLS enablement is based on the current successful CA load (
ca_usable),not a stale “file exists” probe alone. Without
FORCE_TLS, a CA that vanishesor fails mid-load stays plaintext (does not silently flip to HTTPS +
system trust). HTTP aio loads CA from in-memory
cadata; sync HTTP canpersist loaded CA bytes to a temp PEM if the path races away.
FORCE_TLS credential failure is fail-open (documented)
If gRPC/process-trust credentials or
SSLContextconstruction ultimatelyfails, reporters warn and stay plaintext rather than aborting start.
This is intentional best-effort encryption, not fail-closed — spelled out in
docs/en/setup/Intrusive.md/Configuration.md.Diagnostics: mTLS warning without usable CA
Configuring client cert/key without a usable CA now logs the dedicated mTLS
warning even when
FORCE_TLSis off (previously only covered withFORCE_TLS=true).Docs aligned with C-core authority / TLS peer-name
Documented that
:authorityand TLS peer-name checks usegrpc.default_authority(first configured endpoint), not the encodedmulti-backend channel target used for dialing.
mTLS e2e healthcheck
Sharing-server mTLS e2e probes port 11811 (not core 11800).
Test plan
tests/unit/test_tls.py,tests/unit/test_grpc_channel.py(path errors, TOCTOU→plaintext, mTLS-without-CA warning, degrade matrix,
PKCS#1, fork temp safety)
tests/e2e/case/grpc/ssl/,tests/e2e/case/grpc/mtls/(OAP HTTP/REST TLS is server-side only — no real-OAP HTTP mTLS e2e)