-
Notifications
You must be signed in to change notification settings - Fork 2
fix: never leak or persist a user API credential #137
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
0f1d51b
af2a39d
51e9236
28f782f
bfd686a
000af3e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,12 +1,19 @@ | ||
| import time | ||
| from typing import Any | ||
|
|
||
| from pydantic import BaseModel, Field, computed_field | ||
| from pydantic import ( | ||
| BaseModel, | ||
| Field, | ||
| SerializerFunctionWrapHandler, | ||
| computed_field, | ||
| model_serializer, | ||
| ) | ||
|
|
||
| from shared.tasks import TaskEnvelopeTemplate | ||
| from shared.tasks.worker_message import HardwareUsage | ||
|
|
||
| from ..utils.time import now_iso | ||
| from .redact import redact_api, redact_raw_yaml | ||
|
|
||
| TRAINING_TASK_TYPES = { | ||
| "sft", | ||
|
|
@@ -161,6 +168,16 @@ def last_failed_worker(self) -> str | None: | |
| """The most recent worker to have failed this task.""" | ||
| return self.failed_workers[-1] if self.failed_workers else None | ||
|
|
||
| @model_serializer(mode="wrap") | ||
| def _serialize(self, handler: SerializerFunctionWrapHandler) -> dict[str, Any]: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This redacting logic runs on every TaskRecord serialization, which occurs very often, e.g., when task state changes. I suggest redacting only once when persisting the record. |
||
| data = handler(self) | ||
| data["raw_yaml"] = redact_raw_yaml(self.raw_yaml) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| spec = self.task.spec | ||
| api = getattr(spec, "api", None) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use |
||
| if isinstance(api, dict): | ||
| data["task"]["spec"]["api"] = redact_api(api) | ||
| return data | ||
|
|
||
|
|
||
| class TaskInfo(TaskRecord): | ||
| depends_on: list[str] = Field(description="Dependency task IDs.") | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| """Redaction of API credentials before a task record is serialized. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This module can be moved to src/shared/utils. |
||
|
|
||
| The in-memory ``TaskRecord`` keeps the real credential so dispatch works; the | ||
| serializer applies this module so that every dump to Redis is redacted. | ||
|
|
||
| ``raw_yaml`` is re-emitted via ``yaml.safe_dump``, which does not preserve | ||
| comments, key order or original formatting. That is an accepted cost: the field | ||
| is a stored record and is never re-parsed, so losing formatting is fine. If the | ||
| YAML cannot be parsed, the whole field is redacted rather than storing text | ||
| that might contain a key. | ||
| """ | ||
|
|
||
| from typing import Any | ||
|
|
||
| import yaml | ||
|
|
||
| REDACTED = "[REDACTED]" | ||
|
|
||
| # Whole-key matches; a bare substring test would over-redact (e.g. "monkey"). | ||
| _SENSITIVE_KEYS = frozenset( | ||
| { | ||
| "authorization", | ||
| "token", | ||
| "api-key", | ||
| "api_key", | ||
| "apikey", | ||
| "secret", | ||
| "access_token", | ||
| "bearer", | ||
| "x-api-key", | ||
| } | ||
| ) | ||
| _SENSITIVE_SUFFIXES = ("_key", "-key", "_token", "-token") | ||
|
|
||
|
|
||
| def _is_sensitive_key(name: str) -> bool: | ||
| lowered = name.lower() | ||
| if lowered in _SENSITIVE_KEYS: | ||
| return True | ||
| return any(lowered.endswith(suffix) for suffix in _SENSITIVE_SUFFIXES) | ||
|
|
||
|
|
||
| def _redact_value(value: Any) -> Any: | ||
| """Recursively redact credential values by key name at any depth.""" | ||
| if isinstance(value, dict): | ||
| return { | ||
| key: (REDACTED if _is_sensitive_key(str(key)) else _redact_value(val)) | ||
| for key, val in value.items() | ||
| } | ||
| if isinstance(value, list): | ||
| return [_redact_value(item) for item in value] | ||
| return value | ||
|
|
||
|
|
||
| def redact_api(api: dict[str, Any] | None) -> dict[str, Any] | None: | ||
| """Return a copy of an api spec with credential values replaced. | ||
|
|
||
| Only the five credential-bearing locations are touched; the rest of the | ||
| spec is returned unchanged. The original mapping is never mutated. | ||
| """ | ||
| if not isinstance(api, dict): | ||
| return api | ||
| redacted = dict(api) | ||
| for field in ("headers", "params", "body", "json", "data"): | ||
| value = redacted.get(field) | ||
| if isinstance(value, (dict, list)): | ||
| redacted[field] = _redact_value(value) | ||
| return redacted | ||
|
|
||
|
|
||
| def redact_raw_yaml(raw_yaml: str) -> str: | ||
| """Redact credential values from the original workflow YAML text. | ||
|
|
||
| The YAML is parsed and redacted with the same recursive key rule used for | ||
| the parsed spec, then re-emitted. If parsing fails, the whole field is | ||
| replaced with a marker rather than storing un-analysed text. | ||
| """ | ||
| try: | ||
| tree = yaml.safe_load(raw_yaml) | ||
| except yaml.YAMLError: | ||
| return REDACTED | ||
| if tree is None: | ||
| return raw_yaml | ||
| return yaml.safe_dump(_redact_value(tree), sort_keys=False) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,12 +19,13 @@ | |
|
|
||
|
|
||
| class APIExecutor(Executor): | ||
| """Executor that performs a single HTTP request defined by task YAML. | ||
| """Performs a single HTTP request defined by task YAML. | ||
|
|
||
| Uses a class-level connection pool keyed by (base_url, timeout, verify_tls, | ||
| follow_redirects) so that repeated calls to the same endpoint (e.g. a trading | ||
| bot hitting QuantArena every few seconds) reuse the underlying TCP/TLS | ||
| connection instead of paying the handshake cost on every request. | ||
| Defaults to the Nebula endpoint via ``NEBULA_API_BASE_URL`` and authenticates | ||
| with ``NEBULA_API_TOKEN``. ``spec.api.url`` overrides the endpoint and | ||
| ``spec.api.headers`` may supply an ``Authorization`` header directly. A | ||
| custom ``spec.api.url`` requires its own credential: the Nebula token is | ||
| never sent to an endpoint the caller chose. | ||
| """ | ||
|
|
||
| name = "api" | ||
|
|
@@ -99,6 +100,7 @@ def run(self, task: ExecutorTask, out_dir: Path) -> APIResult: | |
| raise ExecutionError("spec.api must be a mapping") | ||
|
|
||
| url = api_cfg.get("url") | ||
| custom_url = url is not None | ||
| if url is None: | ||
| url = os.getenv("NEBULA_API_BASE_URL") | ||
| if not url: | ||
|
|
@@ -110,8 +112,21 @@ def run(self, task: ExecutorTask, out_dir: Path) -> APIResult: | |
| if not isinstance(headers, dict): | ||
| raise ExecutionError("spec.api.headers must be a mapping") | ||
|
|
||
| token = os.getenv("NEBULA_API_TOKEN") | ||
| if token and not any(k.lower() == "authorization" for k in headers): | ||
| has_credential = any(k.lower() == "authorization" for k in headers) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For custom URL, we only check the Authorization header, but some endpoints use different headers, e.g., "X-API-Key". I think we should be more comprehensive. |
||
| if custom_url: | ||
| if not has_credential: | ||
| raise ExecutionError( | ||
| "spec.api.url names a custom endpoint but no credential was " | ||
| "supplied; set an Authorization header. The Nebula token is " | ||
| "never sent to an endpoint the caller chose." | ||
| ) | ||
|
Comment on lines
+117
to
+122
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why must we enforce auth on custom URL? Can we allow opting out? |
||
| elif not has_credential: | ||
| token = os.getenv("NEBULA_API_TOKEN") | ||
| if not token: | ||
| raise ExecutionError( | ||
| "no credential configured: set an Authorization header or " | ||
| "NEBULA_API_TOKEN" | ||
| ) | ||
| headers["Authorization"] = f"Bearer {token}" | ||
|
|
||
| params = api_cfg.get("params") | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method detects only redacted credentials in "headers", but
redact_apiredacts credentials in ("headers", "params", "body", "json", "data"). Also add unit tests to guard against this error.