Skip to content

sec: Support Laminar API key injection to prevent credential leaks #2814

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
simonrosenberg wants to merge 2 commits into from
Closed

sec: Support Laminar API key injection to prevent credential leaks #2814

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b92a7ce
sec: Support Laminar API key injection to prevent credential leaks
Apr 13, 2026
d0740d0
feat: add laminar_span_context parameter to workspace classes
Apr 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 33 additions & 6 deletions openhands-sdk/openhands/sdk/observability/laminar.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,13 @@ def _get_int_env(key: str) -> int | None:
return None
Copy link
Copy Markdown
Collaborator

@all-hands-bot all-hands-bot Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Important - Missing Tests: This is a security-critical feature with zero test coverage.

Required tests:

  1. Verify that when api_key parameter is provided, Laminar initializes correctly
  2. Verify backward compatibility: old code using env vars still works
  3. Verify that the new approach actually prevents the logging issue (if it does)
  4. Verify that warnings appear when expected

Don't just test the happy path - test the actual security property you're claiming to provide.



def maybe_init_laminar():
"""Initialize Laminar if the environment variables are set.
def maybe_init_laminar(api_key: str | None = None):
"""Initialize Laminar if the environment variables are set or api_key is provided.

Args:
api_key: Optional Laminar API key. If provided, used instead of reading from
environment variables. This allows secure credential injection without
exposing secrets in environment variables.

Example configuration:

Expand All @@ -53,8 +58,13 @@ def maybe_init_laminar():
LMNR_HTTP_PORT=8000
LMNR_GRPC_PORT=8001
"""
if should_enable_observability():
if _is_otel_backend_laminar():
if should_enable_observability(api_key=api_key):
if _is_otel_backend_laminar(api_key=api_key):
# If api_key is provided, set it in environment for Laminar initialization
if api_key:
import os
os.environ["LMNR_PROJECT_API_KEY"] = api_key
Copy link
Copy Markdown
Collaborator

@all-hands-bot all-hands-bot Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Critical - Design Flaw: You're still setting os.environ["LMNR_PROJECT_API_KEY"]. This contradicts the entire premise of the PR.

The problem: PR description says credentials leak because "Laminar SDK reads credentials from os.environ[], which gets serialized into logged data structures."

Your solution: Pass the key as a parameter... then set it in os.environ anyway.

Question: How does this prevent the environment from being logged? If any code takes a snapshot of os.environ after maybe_init_laminar() runs, the key will still be there and still leak.

What you've actually done: Changed WHEN the key enters the environment, not WHETHER it enters the environment. That's not a solution to environment serialization.

Better approach: Either:

  1. Patch Laminar SDK to accept API keys directly without reading from environment
  2. Prove with evidence that setting env later actually prevents the specific logging you're trying to avoid
  3. Use a different observability backend that doesn't require environment variables

Copy link
Copy Markdown
Collaborator

@all-hands-bot all-hands-bot Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Critical: This defeats the entire purpose of the PR.

The code still sets os.environ["LMNR_PROJECT_API_KEY"], which means the credential will appear in any code that serializes os.environ (exactly what this PR aims to prevent).

Root cause: Laminar SDK provides a project_api_key parameter that should be used instead:

Laminar.initialize(
    project_api_key=api_key,
    http_port=_get_int_env("LMNR_HTTP_PORT"),
    grpc_port=_get_int_env("LMNR_GRPC_PORT"),
)

This eliminates the need to touch os.environ at all. See help(Laminar.initialize) — the project_api_key parameter exists specifically for this use case.


Laminar.initialize(
http_port=_get_int_env("LMNR_HTTP_PORT"),
grpc_port=_get_int_env("LMNR_GRPC_PORT"),
Expand Down Expand Up @@ -112,7 +122,18 @@ def decorator(func: Callable[P, R]) -> Callable[P, R]:
return decorator


def should_enable_observability():
def should_enable_observability(api_key: str | None = None):
"""Check if observability/tracing should be enabled.

Args:
api_key: Optional Laminar API key. If provided, enables observability
even if no environment variables are set.

Returns:
True if observability should be enabled, False otherwise.
"""
if api_key:
return True
keys = [
"LMNR_PROJECT_API_KEY",
"OTEL_ENDPOINT",
Expand All @@ -126,12 +147,18 @@ def should_enable_observability():
return False


def _is_otel_backend_laminar():
def _is_otel_backend_laminar(api_key: str | None = None):
"""Simple heuristic to check if the OTEL backend is Laminar.

Caveat: This will still be True if another backend uses the same
authentication scheme, and the user uses LMNR_PROJECT_API_KEY
instead of OTEL_HEADERS to authenticate.

Args:
api_key: Optional Laminar API key. If provided, backend is Laminar.
"""
if api_key:
return True
key = get_env("LMNR_PROJECT_API_KEY")
return key is not None and key != ""

Expand Down
20 changes: 20 additions & 0 deletions openhands-workspace/openhands/workspace/apptainer/workspace.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,12 @@ class ApptainerWorkspace(RemoteWorkspace):
default_factory=lambda: ["DEBUG"],
description="Environment variables to forward to the container.",
)
laminar_api_key: str | None = Field(
default=None,
description="Laminar API key for observability tracing. "
"When provided, injected as LMNR_PROJECT_API_KEY in the container. "
"Should NOT be included in forward_env to avoid logging leaks.",
)
mount_dir: str | None = Field(
default=None,
description="Optional host directory to mount into the container.",
Expand Down Expand Up @@ -243,8 +249,22 @@ def _start_container(self) -> None:
env_args: list[str] = []
for key in self.forward_env:
if key in os.environ:
# Skip LMNR_PROJECT_API_KEY if it's in forward_env to avoid log leaks
# Use laminar_api_key field instead
if key == "LMNR_PROJECT_API_KEY":
logger.warning(
"LMNR_PROJECT_API_KEY is in forward_env list. "
Copy link
Copy Markdown
Collaborator

@all-hands-bot all-hands-bot Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion - Same as Docker: Same log spam concern as docker/workspace.py. Consider rate-limiting or making this a migration-period warning.

"This may cause credential leaks in logs. "
"Use the 'laminar_api_key' field instead."
)
continue
env_args += ["--env", f"{key}={os.environ[key]}"]

# Inject Laminar API key directly (not via environment variable)
# This ensures credentials don't appear in logged payloads
if self.laminar_api_key:
env_args += ["--env", f"LMNR_PROJECT_API_KEY={self.laminar_api_key}"]

# Prepare bind mounts
bind_args: list[str] = []
if self.mount_dir:
Expand Down
20 changes: 20 additions & 0 deletions openhands-workspace/openhands/workspace/docker/workspace.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,12 @@ class DockerWorkspace(RemoteWorkspace):
default_factory=lambda: ["DEBUG"],
description="Environment variables to forward to the container.",
)
laminar_api_key: str | None = Field(
default=None,
description="Laminar API key for observability tracing. "
"When provided, injected as LMNR_PROJECT_API_KEY in the container. "
"Should NOT be included in forward_env to avoid logging leaks.",
)
Copy link
Copy Markdown
Collaborator

@all-hands-bot all-hands-bot Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Critical: The laminar_api_key field leaks in serialization.

Tested with workspace.model_dump() — the secret value appears in the output. This defeats the security goal.

Fix: Add Pydantic field attributes to prevent serialization:

laminar_api_key: str | None = Field(
    default=None,
    exclude=True,  # Prevents model_dump() serialization
    repr=False,     # Prevents __repr__ exposure
    description="Laminar API key for observability tracing. "
    "When provided, injected as LMNR_PROJECT_API_KEY in the container. "
    "Should NOT be included in forward_env to avoid logging leaks.",
)

Apply the same fix to apptainer/workspace.py and remote_api/workspace.py.

mount_dir: str | None = Field(
default=None,
description="Optional host directory to mount into the container.",
Expand Down Expand Up @@ -211,8 +217,22 @@ def _start_container(self, image: str, context: Any) -> None:
flags: list[str] = []
for key in self.forward_env:
if key in os.environ:
# Skip LMNR_PROJECT_API_KEY if it's in forward_env to avoid log leaks
# Use laminar_api_key field instead
if key == "LMNR_PROJECT_API_KEY":
logger.warning(
"LMNR_PROJECT_API_KEY is in forward_env list. "
Copy link
Copy Markdown
Collaborator

@all-hands-bot all-hands-bot Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion - Log Spam: This warning will fire every time someone has LMNR_PROJECT_API_KEY in their forward_env, even if they're doing it intentionally for backward compatibility.

Consider: Either make this a one-time warning, or only warn in specific contexts (e.g., when Datadog logging is enabled), or document that users should migrate and remove the warning after a deprecation period.

Continuous log spam isn't helpful - it trains users to ignore warnings.

"This may cause credential leaks in logs. "
"Use the 'laminar_api_key' field instead."
)
continue
flags += ["-e", f"{key}={os.environ[key]}"]

# Inject Laminar API key directly (not via environment variable)
# This ensures credentials don't appear in logged payloads
if self.laminar_api_key:
flags += ["-e", f"LMNR_PROJECT_API_KEY={self.laminar_api_key}"]

for volume in self.volumes:
flags += ["-v", volume]
logger.info(f"Adding volume mount: {volume}")
Expand Down
20 changes: 20 additions & 0 deletions openhands-workspace/openhands/workspace/remote_api/workspace.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,12 @@ class APIRemoteWorkspace(RemoteWorkspace):
default_factory=list,
description="Environment variable names to forward from host to runtime.",
)
laminar_api_key: str | None = Field(
default=None,
description="Laminar API key for observability tracing. "
"When provided, injected as LMNR_PROJECT_API_KEY in the runtime environment. "
"Should NOT be included in forward_env to avoid logging leaks.",
)

_runtime_id: str | None = PrivateAttr(default=None)
_runtime_url: str | None = PrivateAttr(default=None)
Expand Down Expand Up @@ -192,8 +198,22 @@ def _start_runtime(self) -> None:
environment: dict[str, str] = {}
for key in self.forward_env:
if key in os.environ:
# Skip LMNR_PROJECT_API_KEY if it's in forward_env to avoid log leaks
# Use laminar_api_key field instead
if key == "LMNR_PROJECT_API_KEY":
logger.warning(
"LMNR_PROJECT_API_KEY is in forward_env list. "
"This may cause credential leaks in logs. "
"Use the 'laminar_api_key' field instead."
)
continue
environment[key] = os.environ[key]

# Inject Laminar API key directly (not via environment variable)
# This ensures credentials don't appear in logged payloads
if self.laminar_api_key:
environment["LMNR_PROJECT_API_KEY"] = self.laminar_api_key
Copy link
Copy Markdown
Collaborator

@all-hands-bot all-hands-bot Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Important - Missing Evidence: The PR claims this prevents credential leaks in "workspace initialization payloads (logged to Datadog)" and "runtime API request logs (/start endpoint)".

Where's the proof? Show me:

  1. Before: Log output with the leaked credential
  2. After: Same scenario with the credential masked/absent
  3. Test coverage that verifies this behavior

Security claims without evidence are just wishes. Did you actually verify this prevents the logging you mentioned in OpenHands/evaluation#390?

Copy link
Copy Markdown
Collaborator

@all-hands-bot all-hands-bot Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Important: The comment claims "credentials don't appear in logged payloads," but this is misleading.

The credential is added to the environment dict, which is then:

  1. Included in the payload dict (line 222)
  2. Sent over HTTP to the runtime API (line 245-251)
  3. May be logged by the runtime API server, proxies, or middleware

Additionally, line 243 logs environment_keys=sorted(environment), which reveals that LMNR_PROJECT_API_KEY is present.

The credential is still being passed as an environment variable — just injected at a different point. The proper fix is to use Laminar's project_api_key parameter and avoid environment variables entirely.


# For binary target, use the standalone binary
payload: dict[str, Any] = {
"image": self.server_image,
Expand Down
Loading
Loading