Skip to content

fix: implement OTEL_EXPORTER_OTLP_INSECURE configuration setting#4711

Draft
kimjune01 wants to merge 5 commits into
IBM:mainfrom
kimjune01:fix/otel-insecure-setting-4644
Draft

fix: implement OTEL_EXPORTER_OTLP_INSECURE configuration setting#4711
kimjune01 wants to merge 5 commits into
IBM:mainfrom
kimjune01:fix/otel-insecure-setting-4644

Conversation

@kimjune01
Copy link
Copy Markdown

Summary

Fixes #4644 by properly implementing the OTEL_EXPORTER_OTLP_INSECURE configuration setting for OpenTelemetry HTTP and gRPC exporters.

Root Cause

The insecure parameter was never passed to OTLP exporter constructors, despite being defined in config.py and documented. This caused SSL certificate verification errors when users attempted to export traces/metrics to servers with self-signed certificates by setting OTEL_EXPORTER_OTLP_INSECURE=true.

Additionally, HTTP exporters don't accept an insecure parameter—they determine TLS usage from the endpoint URL scheme (http:// vs https://).

Changes

HTTP Exporter Fix

  • Rewrite endpoint URL scheme from https:// to http:// when insecure=true is set
  • HTTP exporters now respect the OTEL_EXPORTER_OTLP_INSECURE setting without requiring a direct insecure parameter

gRPC Exporter

  • Pass insecure parameter to gRPC exporter constructors when configured

Test Coverage

  • Added comprehensive tests for HTTP/gRPC span and metric exporters with insecure flag combinations
  • Added environment variable cleanup to prevent test pollution

Bug Fixes

During QA, Gemini found two critical bugs:

  1. HTTP exporter TLS logic: Original implementation incorrectly tried to pass insecure parameter to HTTP exporters, which don't accept it. Fixed by rewriting URL schemes instead.
  2. Test pollution: Missing OTEL_AVAILABLE patch in new tests. Added cleanup to prevent cross-test contamination.

Testing

uv run pytest tests/unit/mcpgateway/test_observability.py
# 76 passed, 1 skipped

Verified:

  • HTTP exporters correctly use http:// when insecure=true
  • gRPC exporters receive insecure=true parameter
  • Existing tests remain green

@kimjune01
Copy link
Copy Markdown
Author

Follow-up to my own re-QA on this PR. The previous revision had a security bug I want to acknowledge and explain the fix.

What was wrong

The HTTP exporter branch in mcpgateway/observability.py mutated the OTLP URL scheme based on OTEL_EXPORTER_OTLP_INSECURE in both directions:

  • insecure=false upgraded http:// -> https://, breaking standard local-collector setups like http://localhost:4318.
  • insecure=true downgraded https:// -> http://. Because the config default for otel_exporter_otlp_insecure is True (config.py:2402), this branch fired by default. Any user who only set OTEL_EXPORTER_OTLP_ENDPOINT=https://... and left INSECURE unset would have had TLS silently stripped, sending traces and auth headers (e.g., Langfuse basic auth) over plaintext.

The fix (commit e7007c3)

Per the OTEL spec (OTLP/HTTP exporter), the URL scheme is the source of truth for HTTP transport TLS. The insecure flag is meaningful only for OTLP/gRPC. So I dropped scheme mutation on the HTTP path entirely. The gRPC branch still passes insecure= through unchanged.

I also tightened the :4317 -> :4318 port rewrite so it does not double-append /v1/traces for endpoints like http://proxy:4317/v1/traces, and strips trailing slashes before appending.

Test changes:

  • test_init_telemetry_otlp_http_insecure_true -> ..._true_preserves_https (inverted; asserts https stays https)
  • test_init_telemetry_otlp_http_insecure_false -> ..._false_preserves_http (inverted; asserts http stays http -- this previously codified the broken upgrade direction)
  • New: test_init_telemetry_otlp_http_insecure_unset_preserves_https covers the actual risk case where the default insecure=True would have triggered the downgrade.

77 passed, 1 skipped (pre-existing).

Review trail

Re-QA via adversarial review: gemini-3.1-pro round 1 surfaced 4 findings (the two TLS-stripping bugs above, the masked test, and the brittle port rewrite). Round 2 against the same reviewer converged to "ZERO BUGS." Codex was unavailable (quota exhausted until 2026-05-17), so this round used Gemini only per fallback policy.

Happy to break this into separate commits if you would prefer that for review.

@kimjune01 kimjune01 marked this pull request as draft May 17, 2026 20:08
kimjune01 and others added 5 commits May 18, 2026 12:28
The OTEL_EXPORTER_OTLP_INSECURE setting was defined in config.py and documented but not used by the OTLP exporter initialization code. This caused SSL verification errors for users trying to export telemetry to servers with self-signed certificates (e.g., Langfuse on OpenShift).

Changes:
- Pass cfg.otel_exporter_otlp_insecure to both GRPC and HTTP OTLP exporters
- Remove outdated comment about exporter compatibility
- Add unit tests to verify insecure parameter is correctly passed

Fixes IBM#4644

Signed-off-by: June Kim <[email protected]>
- HTTP exporter doesn't accept insecure kwarg, uses http:// vs https://
- Rewrite endpoint scheme based on OTEL_EXPORTER_OTLP_INSECURE setting
- Update tests to verify URL scheme instead of parameter
- Add OTEL_EXPORTER_OTLP_INSECURE and PROTOCOL to teardown cleanup

Signed-off-by: June Kim <[email protected]>
…re flag

The HTTP exporter branch previously rewrote the URL scheme based on
OTEL_EXPORTER_OTLP_INSECURE in both directions:

  * insecure=false upgraded http:// -> https:// (broke standard local
    collector setups like http://localhost:4318)
  * insecure=true downgraded https:// -> http://; because the config
    default is True, any user who only set
    OTEL_EXPORTER_OTLP_ENDPOINT=https://... had TLS silently stripped,
    sending traces and auth headers (e.g., Langfuse basic auth) in
    plaintext

Per the OTEL spec (OTLP/HTTP), the URL scheme is the source of truth for
transport TLS. The `insecure` flag is meaningful only for OTLP/gRPC,
where it is still passed through unchanged. Drop scheme mutation on the
HTTP path entirely.

Also tighten the :4317 -> :4318 port rewrite: only append `/v1/traces`
when the endpoint does not already target a `/v1/` resource, and strip
trailing slashes before appending. This fixes path double-appending for
endpoints like `http://proxy:4317/v1/traces`.

Tests updated:

  * test_init_telemetry_otlp_http_insecure_true_preserves_https
    (renamed and inverted from ..._true; now asserts https:// is kept
    verbatim even when insecure=true)
  * test_init_telemetry_otlp_http_insecure_false_preserves_http
    (renamed and inverted from ..._false; previously codified the
    broken http -> https upgrade)
  * test_init_telemetry_otlp_http_insecure_unset_preserves_https
    (new; covers the actual risk case where the default insecure=True
    would have triggered the downgrade)

Signed-off-by: June Kim <[email protected]>
@kimjune01 kimjune01 force-pushed the fix/otel-insecure-setting-4644 branch from e7007c3 to b566ce5 Compare May 18, 2026 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG]: OTEL_EXPORTER_OTLP_INSECURE configuration setting is defined but not used

1 participant