security: Require user auth secret everywhere#2458
Open
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
|
topher-lo
temporarily deployed
to
internal-registry-ci
GitHub Actions
Inactive
topher-lo
had a problem deploying
to
internal-registry-ci
GitHub Actions
Failure
|
✅ No security or compliance issues detected. Reviewed everything up to 8111d03. Security Overview
Detected Code Changes
|
topher-lo
changed the title
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 17 files
Confidence score: 3/5
- There is a concrete user-impacting risk in
tracecat/config.py: usinginfor the OIDC gate can trigger a startup crash in mixed auth setups like{BASIC, OIDC}. - Because this is a high-severity, high-confidence finding (7/10, 9/10) affecting app initialization, the PR carries moderate merge risk until the condition is tightened to an exact set check.
- Pay close attention to
tracecat/config.py- the auth-type gate logic should use exact matching to avoid crashing when multiple auth modes are configured.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tracecat/config.py">
<violation number="1" location="tracecat/config.py:400">
P1: Use an exact set match (`== {AuthType.OIDC}`) instead of a membership check (`in`) for the OIDC config gate. In mixed auth configurations (e.g., `{BASIC, OIDC}`), this membership check will crash the app at startup if OIDC vars are missing, preventing BASIC auth from being usable. The old code intentionally used an exact match so BASIC remains available and OIDC initialization fails later only when actually exercised.
(Based on your team's feedback about requiring OIDC_ISSUER only when auth types is exactly OIDC.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| ) | ||
| """Executor JWT TTL in seconds (default: 900 seconds).""" | ||
|
|
||
| if AuthType.OIDC in TRACECAT__AUTH_TYPES: |
Contributor
There was a problem hiding this comment.
P1: Use an exact set match (== {AuthType.OIDC}) instead of a membership check (in) for the OIDC config gate. In mixed auth configurations (e.g., {BASIC, OIDC}), this membership check will crash the app at startup if OIDC vars are missing, preventing BASIC auth from being usable. The old code intentionally used an exact match so BASIC remains available and OIDC initialization fails later only when actually exercised.
(Based on your team's feedback about requiring OIDC_ISSUER only when auth types is exactly OIDC.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tracecat/config.py, line 400:
<comment>Use an exact set match (`== {AuthType.OIDC}`) instead of a membership check (`in`) for the OIDC config gate. In mixed auth configurations (e.g., `{BASIC, OIDC}`), this membership check will crash the app at startup if OIDC vars are missing, preventing BASIC auth from being usable. The old code intentionally used an exact match so BASIC remains available and OIDC initialization fails later only when actually exercised.
(Based on your team's feedback about requiring OIDC_ISSUER only when auth types is exactly OIDC.) </comment>
<file context>
@@ -385,12 +391,48 @@ def _parse_auth_types() -> set[AuthType]:
)
"""Executor JWT TTL in seconds (default: 900 seconds)."""
+if AuthType.OIDC in TRACECAT__AUTH_TYPES:
+ if not OIDC_ISSUER:
+ raise ValueError(
</file context>
Suggested change
| if AuthType.OIDC in TRACECAT__AUTH_TYPES: | |
| if TRACECAT__AUTH_TYPES == {AuthType.OIDC}: |
topher-lo
temporarily deployed
to
internal-registry-ci
GitHub Actions
Inactive
topher-lo
had a problem deploying
to
internal-registry-ci
GitHub Actions
Failure
Contributor
|
Found 8 test failures on Blacksmith runners: Failures
|
topher-lo
temporarily deployed
to
internal-registry-ci
GitHub Actions
Inactive
topher-lo
temporarily deployed
to
internal-registry-ci
GitHub Actions
Inactive
Contributor
There was a problem hiding this comment.
2 issues found across 7 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/unit/test_oidc_config.py">
<violation number="1" location="tests/unit/test_oidc_config.py:154">
P2: This test now expects config reload to succeed with USER_AUTH_SECRET unset, which contradicts the new “required everywhere” secret policy and removes coverage for the fail-fast check.</violation>
<violation number="2" location="tests/unit/test_oidc_config.py:190">
P2: This test now asserts that missing core secrets are acceptable, which weakens coverage for the required-secret checks and can mask regressions in the fail-fast enforcement.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Comment on lines
+190
to
+198
| def test_config_allows_missing_core_tracecat_secrets( | ||
| monkeypatch, var_name: str | ||
| ) -> None: | ||
| with monkeypatch.context() as env: | ||
| env.setenv("TRACECAT__AUTH_TYPES", "saml") | ||
| env.delenv(var_name, raising=False) | ||
| importlib.reload(config) | ||
|
|
||
| importlib.reload(config) |
Contributor
There was a problem hiding this comment.
P2: This test now asserts that missing core secrets are acceptable, which weakens coverage for the required-secret checks and can mask regressions in the fail-fast enforcement.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/unit/test_oidc_config.py, line 190:
<comment>This test now asserts that missing core secrets are acceptable, which weakens coverage for the required-secret checks and can mask regressions in the fail-fast enforcement.</comment>
<file context>
@@ -180,25 +179,50 @@ def test_config_requires_oidc_client_secret_when_oidc_enabled(monkeypatch) -> No
)
-def test_config_requires_core_tracecat_secrets(
- monkeypatch, var_name: str, expected_message: str
+def test_config_allows_missing_core_tracecat_secrets(
+ monkeypatch, var_name: str
) -> None:
</file context>
Suggested change
| def test_config_allows_missing_core_tracecat_secrets( | |
| monkeypatch, var_name: str | |
| ) -> None: | |
| with monkeypatch.context() as env: | |
| env.setenv("TRACECAT__AUTH_TYPES", "saml") | |
| env.delenv(var_name, raising=False) | |
| importlib.reload(config) | |
| importlib.reload(config) | |
| def test_config_requires_core_tracecat_secrets( | |
| monkeypatch, var_name: str | |
| ) -> None: | |
| with monkeypatch.context() as env: | |
| env.setenv("TRACECAT__AUTH_TYPES", "saml") | |
| env.delenv(var_name, raising=False) | |
| with pytest.raises(KeyError, match=f"{var_name} must be set"): | |
| importlib.reload(config) | |
| importlib.reload(config) |
| importlib.reload(config) | ||
|
|
||
|
|
||
| def test_config_allows_missing_user_auth_secret_when_auth_type_does_not_use_it( |
Contributor
There was a problem hiding this comment.
P2: This test now expects config reload to succeed with USER_AUTH_SECRET unset, which contradicts the new “required everywhere” secret policy and removes coverage for the fail-fast check.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/unit/test_oidc_config.py, line 154:
<comment>This test now expects config reload to succeed with USER_AUTH_SECRET unset, which contradicts the new “required everywhere” secret policy and removes coverage for the fail-fast check.</comment>
<file context>
@@ -150,15 +151,13 @@ def test_config_keeps_oauth_aliases_for_oidc_credentials(monkeypatch) -> None:
-def test_config_requires_user_auth_secret_unconditionally(monkeypatch) -> None:
+def test_config_allows_missing_user_auth_secret_when_auth_type_does_not_use_it(
+ monkeypatch,
+) -> None:
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This change hardens Tracecat's core secret handling by making
USER_AUTH_SECRETrequired everywhere instead of conditionally optional. It also keeps offline OpenAPI generation deterministic by bootstrapping explicit tooling-only placeholder values instead of relying on a developer.envor real runtime secrets.Problem
Older deployments could leave
USER_AUTH_SECRETunset or empty. That weakened password reset, email verification, and OAuth/OIDC state-token signing, while also leaving the enforcement story inconsistent across the application, Fargate Terraform, and operator documentation.At the same time, strict import-time config validation started blocking OpenAPI generation because the generator imports the application and therefore inherited the same required secret checks, even though schema generation does not need real secrets.
Root cause
USER_AUTH_SECRETwas treated as optional in parts of the stack and only effectively enforced in specific auth modes. Terraform inputs and docs reflected that older assumption, which allowed misconfigured environments to survive until a sensitive auth path was exercised. Tooling paths were also coupled too tightly to runtime configuration.Fix
The application now fails fast when any of the four core secrets are empty:
USER_AUTH_SECRET,TRACECAT__DB_ENCRYPTION_KEY,TRACECAT__SIGNING_SECRET, andTRACECAT__SERVICE_KEY.Fargate now treats
user_auth_secret_arnas a required input with nodefault = null, and the ECS secret wiring is unconditional soterraform planfails immediately when the secret ARN is missing.The self-hosting docs were updated to describe supported deployment options, clarify the required secrets, warn that all of them must be cryptographically secure and non-empty, and position the Enterprise Kubernetes deployment as the path for larger installs.
OpenAPI generation now sets process-local fake values before importing the app, which preserves strict runtime enforcement without requiring real secrets for offline schema generation.
Validation
I validated the config and deployment changes with:
uv run pytest tests/unit/test_oidc_config.pyuv run ruff check tracecat/config.py tests/unit/test_oidc_config.py tests/conftest.pyuv run python scripts/openapi/generate_openapi.pyterraform fmt deployments/fargate/variables.tf deployments/fargate/modules/ecs/variables.tf deployments/fargate/modules/ecs/secrets.tf deployments/fargate/main.tfNotes for operators
Rotating
USER_AUTH_SECRETdoes not invalidate existing logged-in sessions because normal sessions are DB-backed opaque tokens. It does invalidate outstanding password reset tokens, email verification tokens, and in-flight OAuth/OIDC state tokens.Summary by cubic
Require
USER_AUTH_SECRETat API startup and make it mandatory in Fargate, while scoping other secret checks to auth-specific runtime paths so tooling isn’t blocked. OpenAPI generation now seeds process-local placeholders, and Docker Compose passes core secrets to themigrationsservice.Refactors
USER_AUTH_SECRETis missing;UserManagerenforces it only whenbasicis enabled.OIDC_ISSUER,OIDC_CLIENT_ID, andOIDC_CLIENT_SECRETwhenoidcis enabled; ignore for other modes.user_auth_secret_arn(ECS wiring is unconditional;deployments/fargate/scripts/create-aws-secrets.shcreates it). OpenAPI generator seeds fake values before import. Docs add a self-hosting overview and clarify required secrets;.env.examplemarks required fields.Migration
USER_AUTH_SECRETin all environments. Ensure all required secrets are non-empty.USER_AUTH_SECRETand passTF_VAR_user_auth_secret_arn;terraform planwill fail if missing.Written for commit 8111d03. Summary will update on new commits.