fix: require authentication by default in standalone mode#431
fix: require authentication by default in standalone mode#431ambient-code[bot] wants to merge 10 commits into
Conversation
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@ambient-code you need to update the e2e tests most probably... |
|
Updated the e2e tests in
Commit: a25a70f |
Status UpdateReviewed the PR and its current state: Comments: The only actionable comment was from @mangelajo requesting e2e test updates. This was addressed in commit a25a70f which:
CI: No failures so far. Several checks have passed (lint-python, typos, pytest-matrix ubuntu 3.11/3.12, e2e-compat-old-client). Some checks are still pending (e2e-tests, macOS pytest-matrix runs). Will need to confirm once those complete. Code review notes:
No fixes required -- waiting on remaining CI checks to complete. |
| --tls-grpc-listener "$LISTENER_PORT" \ | ||
| --tls-grpc-insecure 2>"${BATS_TEST_TMPDIR}/exporter.log" & | ||
| LISTENER_PID=$! | ||
| echo "$LISTENER_PID" > "${BATS_TEST_TMPDIR}/exporter.pid" |
There was a problem hiding this comment.
How is the lifecycle of this exporter handled , will something kill it?
There was a problem hiding this comment.
The exporter's lifecycle is managed by the existing teardown() function (line 96), which calls stop_exporter(). The test writes the PID to ${BATS_TEST_TMPDIR}/exporter.pid (line 193), and stop_exporter() reads that file, kills the process, waits for it, and removes the PID file. This is the same mechanism used by all other tests in this file via _start_exporter().
Review feedback addressedAddressed 3 review comments from @raballew (commit 3deeba7): 1. Missing negative test for auto-generated passphrase (MEDIUM) -- Agreed and fixed. 2. Pre-existing misleading warning (MEDIUM) -- Agreed and fixed. 3. Fragile passphrase extraction (LOW) -- Acknowledged as a nit. The coupling between the CI: All checks passing prior to this push. |
ambient-code
Bot
force-pushed
the
fix/356-standalone-auth-default
branch
from
3deeba7 to
b60ee8e
Compare
Rebased onto main to resolve merge conflictThe Resolution: Ported all auth-by-default e2e test changes to the new Go test framework:
The |
raballew
left a comment
raballew
left a comment
There was a problem hiding this comment.
All three previous review concerns have been addressed.
-
Negative test for auto-generated passphrase - resolved. The test now verifies that a wrong passphrase is rejected after auto-generation (lines 211-214 of direct_listener_test.go).
-
Misleading warning reworded - resolved. The warning now reads "passphrase authentication is active but TLS is disabled" instead of the previous inaccurate phrasing.
-
Fragile passphrase extraction - acknowledged. The Go test uses
SplitNwith a non-empty assertion, which is acceptable.
One minor observation noted inline. The core logic is sound: auto-generation is scoped to standalone mode only, validation is correct, and the security model (hmac.compare_digest, tiered warnings) is well implemented.
AI-generated review
Review feedback addressed (round 3)Addressed 1 review comment from @raballew (commit 2e6e038): Shared stderr buffer accumulates data across tests (LOW) -- Agreed and fixed. CI: All checks were passing prior to this push. |
|
@ambient-code rebase onto main |
Standalone mode previously defaulted to no passphrase, exposing an unauthenticated gRPC server. Now a random passphrase is auto-generated when --passphrase is not provided. Users must pass --unsafe-no-auth to explicitly disable authentication. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Existing tests that ran without authentication now use --unsafe-no-auth since the exporter auto-generates a passphrase when neither --passphrase nor --unsafe-no-auth is provided. Also adds tests for auto-generated passphrase output, --unsafe-no-auth flag, and mutual exclusivity of --passphrase and --unsafe-no-auth. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ading warning Address review feedback from @raballew: - Add negative test verifying wrong passphrase is rejected when using auto-generated passphrase (closes test coverage gap) - Reword pre-existing warning to clarify that passphrase auth IS active even without TLS, but the passphrase is transmitted in plaintext Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…tion Reset pt.logs in StopAll() so that stale stderr data from earlier tests does not persist across test boundaries, avoiding confusion when debugging test failures. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ambient-code
Bot
force-pushed
the
fix/356-standalone-auth-default
branch
from
2e6e038 to
08166d9
Compare
|
Rebased onto main. The 4 PR commits applied cleanly with no conflicts.
CI should re-run on the new head. |
Add tests for the new auth-related validation and warning paths in the `run` command to satisfy the diff-coverage >= 80% CI gate. Tests cover: - --passphrase and --unsafe-no-auth mutual exclusivity - --tls-grpc-listener TLS mode requirement - auto-generated passphrase printed to stderr - --unsafe-no-auth + --tls-grpc-insecure "completely unprotected" warning - --unsafe-no-auth with TLS "running without authentication" warning - --unsafe-no-auth without --tls-grpc-listener error Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
CI fix: diff-coverage below 80% thresholdThe Fix (commit 0062406): Added 6 unit tests to
This should bring diff-coverage to 100% on the changed lines. |
CI FixesThree CI failures detected after the rebase:
Investigating and fixing now. |
- Move tempfile and os imports to top-level for proper import sorting (fixes ruff I001) - Remove invalid mix_stderr parameter from CliRunner (fixes ty unknown-argument) These errors were introduced after rebase and prevented CI from passing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
CI Fixes AppliedFixed lint and type-check errors in
Commit: ebe5b88 Note on test failure: The CI should now pass for the changes introduced by this PR. |
| if passphrase: | ||
| click.echo( | ||
| "WARNING: --passphrase has no effect without TLS; " | ||
| "WARNING: passphrase authentication is active but TLS is disabled; " |
There was a problem hiding this comment.
[MEDIUM] The reworded TLS warning here has no test coverage. Every other warning path in this PR has a dedicated test. Consider adding one that starts with --passphrase X --tls-grpc-insecure and asserts stderr contains "authentication is active but TLS is disabled".
AI-generated, human reviewed
There was a problem hiding this comment.
Agreed -- fixed in db65a20. Moved the TLS+passphrase warning from _handle_child to the run() function (alongside the other warning paths at lines 325-336), so it is exercisable through CliRunner without forking. Added test_passphrase_with_tls_insecure_warns_plaintext which invokes with --passphrase X --tls-grpc-insecure and asserts stderr contains "authentication is active but TLS is disabled".
| def test_auto_generated_passphrase_printed_to_stderr(self): | ||
| """When no --passphrase and no --unsafe-no-auth, a passphrase is auto-generated and printed.""" | ||
| result = self._invoke([ | ||
| "--exporter-config", "/dev/null", | ||
| "--tls-grpc-listener", "1234", | ||
| "--tls-grpc-insecure", | ||
| ]) | ||
| assert result.exit_code == 0 | ||
| assert "Generated random passphrase" in result.stderr |
There was a problem hiding this comment.
[MEDIUM] This test checks that the "Generated random passphrase" message shows up, but never verifies that the generated value actually gets forwarded to _serve_with_exc_handling. The mock returns 0 without inspecting its arguments, so a bug that generates a passphrase but passes None to the serve function would slip through. Capturing mock_serve.call_args and asserting the passphrase kwarg is a non-empty string would close this gap.
AI-generated, human reviewed
There was a problem hiding this comment.
Good catch -- fixed in db65a20. The test now creates a named MagicMock for _serve_with_exc_handling, then inspects mock_serve.call_args after invocation to assert that the forwarded passphrase is a non-empty string. This closes the gap where the passphrase could be generated but silently dropped before reaching the serve function.
| for _, line := range strings.Split(stderrBuf.String(), "\n") { | ||
| if strings.Contains(line, "Generated random passphrase") { | ||
| parts := strings.SplitN(line, ": ", 2) | ||
| if len(parts) == 2 { | ||
| generatedPassphrase = strings.TrimSpace(parts[1]) | ||
| } | ||
| break | ||
| } |
There was a problem hiding this comment.
[LOW] The passphrase extraction logic splits on ": " to grab the auto-generated passphrase from stderr. This couples the test to the exact message format. Since both the format and the extraction are introduced in the same PR, and the test guards against empty extraction with Expect(generatedPassphrase).NotTo(BeEmpty()), the risk is acceptable. Just worth noting that any future rewording of that log line will need to update this test too.
AI-generated, human reviewed
There was a problem hiding this comment.
Acknowledged. The coupling is acceptable since both the message format and extraction are maintained in the same PR, and the Expect(generatedPassphrase).NotTo(BeEmpty()) guard catches format mismatches at test time. If the log message format changes in the future, the test failure will clearly point to the extraction logic.
| passphrase = secrets.token_urlsafe(32) | ||
| click.echo( | ||
| f"Generated random passphrase (use --passphrase to set your own): {passphrase}", | ||
| err=True, | ||
| ) |
There was a problem hiding this comment.
[LOW] The auto-generated passphrase is printed to stderr, making it visible in terminal scrollback and any log files that capture stderr. This is intentional for a lab tool, but on shared systems it could be a concern. Supporting a JMP_PASSPHRASE environment variable as an alternative input mechanism would give users a way to avoid this exposure when needed.
AI-generated, human reviewed
There was a problem hiding this comment.
Good suggestion. A JMP_PASSPHRASE environment variable would be a natural complement to --passphrase for shared/CI environments where stderr logging is a concern. That said, it's a distinct feature from the auth-by-default change in this PR. I'd suggest tracking it as a follow-up issue rather than expanding the scope here.
Address review feedback from @raballew: - Move TLS+passphrase warning from _handle_child to run() so it is testable alongside the other warning paths - Add test_passphrase_with_tls_insecure_warns_plaintext - Strengthen test_auto_generated_passphrase_printed_to_stderr to verify that the generated passphrase is actually forwarded to _serve_with_exc_handling via call_args inspection Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Review feedback addressed (round 4)Addressed 4 review comments from @raballew (commit db65a20): 1. TLS warning has no test coverage (MEDIUM) -- Agreed and fixed. 2. Auto-generated passphrase not verified to be forwarded (MEDIUM) -- Agreed and fixed. 3. Passphrase extraction coupled to message format (LOW) -- Acknowledged. The coupling is acceptable since both sides are maintained in this PR, and the 4. Passphrase visible in stderr/logs (LOW) -- Acknowledged as a valid concern for shared systems. Suggested tracking CI: All checks were passing prior to this push. |
CI Fix: lint-python failure (C901 complexity)The Fix plan: Extract the standalone-mode validation logic (mutual exclusivity checks, auto-passphrase generation, and warning emission) into a dedicated |
The run() function exceeded the C901 cyclomatic complexity threshold (11 > 10) due to the auth-by-default validation branches. Extract mutual-exclusivity checks, auto-passphrase generation, and warning emission into a dedicated _validate_standalone_auth() helper. No behavioral change -- same validation logic, same tests pass. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
CI Fix AppliedFixed the Problem: The Fix: Extracted the standalone auth validation logic into a new No behavioral change -- the same validation logic runs in the same order, and all existing tests pass. Verified locally with |
| loading are mocked so the command can reach the auth-validation code | ||
| without actually forking or reading real config files. | ||
| """ | ||
| runner = click.testing.CliRunner() |
There was a problem hiding this comment.
[HIGH] CliRunner() is instantiated with the default mix_stderr=True here (and again at line 254). In Click 8.3.1, accessing result.stderr when mix_stderr=True raises ValueError("stderr is not separately captured"). Five assertions across four tests access result.stderr (lines 269, 287, 298, 320, 321), so those tests will crash before they can verify the warning and auto-generation paths they target.
Fix: pass mix_stderr=False to both CliRunner() calls:
runner = click.testing.CliRunner(mix_stderr=False)AI-generated, human reviewed
There was a problem hiding this comment.
Agreed. Will pass mix_stderr=False to both CliRunner() calls.
| if tls_insecure: | ||
| if passphrase: | ||
| click.echo( | ||
| "WARNING: --passphrase has no effect without TLS; " | ||
| "the passphrase will be transmitted in plaintext", | ||
| err=True, | ||
| ) | ||
| pass # Warning already emitted by run() |
There was a problem hiding this comment.
[MEDIUM] This if tls_insecure: pass branch is dead code. It exists only to keep the elif on line 108 syntactically valid, but that elif works fine as a standalone if. The pass with a comment is also the only uncovered line in the diff.
Suggested fix: remove the dead branch and convert elif to if:
if tls_cert and tls_key:
tls_credentials = _tls_server_credentials(tls_cert, tls_key)The tls_insecure case already falls through to tls_credentials = None on line 105.
AI-generated, human reviewed
There was a problem hiding this comment.
Agreed. Will remove the dead if tls_insecure: pass branch and convert the elif to a standalone if.
There was a problem hiding this comment.
Fixed in 7a6a596. The dead if tls_insecure: pass branch has been removed and the elif converted to a standalone if.
| """Validate auth options for standalone mode and return the effective passphrase. | ||
|
|
||
| Handles mutual-exclusivity checks, auto-generation of passphrases, and | ||
| warning emission. Returns the passphrase to use (may be auto-generated). | ||
| """ |
There was a problem hiding this comment.
[MEDIUM] This docstring restates what the function name and parameters already convey. The project's code style prefers self-explanatory names over comments. Similarly, the inline comment at line 262 (# Auto-generate a passphrase...) restates the if condition. Consider removing both to keep the code consistent with the project convention.
AI-generated, human reviewed
There was a problem hiding this comment.
Agreed. Will remove the docstring from _validate_standalone_auth and the inline comment at line 262.
There was a problem hiding this comment.
Fixed in 7a6a596. Removed the docstring and inline narration comment to match project convention.
| ), | ||
| ) | ||
|
|
||
| # Stack all context-manager patches |
There was a problem hiding this comment.
[MEDIUM] Several section-divider and narration comments throughout this file (lines 214, 223, 246, 276, 326) restate what the code and test names already communicate. The project convention treats comments as a last resort when code cannot be made self-explanatory. Consider removing them.
AI-generated, human reviewed
There was a problem hiding this comment.
Agreed. Will remove section-divider and narration comments from the test file.
There was a problem hiding this comment.
Fixed in 7a6a596. Removed section-divider and narration comments throughout the test file.
| sys.exit(1) # should never happen | ||
|
|
||
|
|
||
| def _validate_standalone_auth(passphrase, unsafe_no_auth, tls_insecure): |
There was a problem hiding this comment.
[MEDIUM] _validate_standalone_auth has no type annotations on its parameters or return value. Since this is a validation boundary that returns either str or None, type annotations would make the contract clear to callers.
Suggested fix:
def _validate_standalone_auth(
passphrase: str | None, unsafe_no_auth: bool, tls_insecure: bool
) -> str | None:AI-generated, human reviewed
There was a problem hiding this comment.
Agreed. Will add type annotations to _validate_standalone_auth.
There was a problem hiding this comment.
Fixed in 7a6a596. Added type annotations: def _validate_standalone_auth(passphrase: str | None, unsafe_no_auth: bool, tls_insecure: bool) -> str | None.
| Also verify that the generated passphrase is actually forwarded to | ||
| ``_serve_with_exc_handling`` (not silently dropped). | ||
| """ | ||
| runner = click.testing.CliRunner() |
There was a problem hiding this comment.
[LOW] _validate_standalone_auth has five distinct branches but is only tested through runner.invoke(run, args), adding Click argument parsing and exception handling as extra layers. If a test fails, it is harder to tell whether the issue is in the validation logic or Click wiring. Consider adding direct unit tests calling _validate_standalone_auth with each branch combination and asserting on return values and raised exceptions.
AI-generated, human reviewed
There was a problem hiding this comment.
Agreed. Will add direct unit tests for _validate_standalone_auth that exercise each branch in isolation without the Click wiring layer.
There was a problem hiding this comment.
Fixed in 7a6a596. Added direct unit tests calling _validate_standalone_auth with each branch combination, asserting on return values and raised exceptions.
| click.echo( | ||
| f"Generated random passphrase (use --passphrase to set your own): {passphrase}", | ||
| err=True, | ||
| ) |
There was a problem hiding this comment.
[LOW] The auto-generated passphrase is printed to stderr via click.echo(..., err=True). In containerized deployments, stderr is captured by log drivers and forwarded to log aggregation systems, where the passphrase may have broader read access than the exporter console. Consider adding a --passphrase-file option for production deployments, or documenting the log-exposure risk in --help text.
AI-generated, human reviewed
There was a problem hiding this comment.
Acknowledged. This is a valid concern for containerized deployments. Adding --passphrase-file would expand the scope of this PR significantly. Will track as a follow-up issue.
There was a problem hiding this comment.
Acknowledged. A --passphrase-file option would be valuable for containerized deployments, but it is out of scope for this PR. The current stderr approach is appropriate for the lab/development use case this PR targets.
| "--passphrase", | ||
| "passphrase", | ||
| default=None, | ||
| help="Require this passphrase from clients connecting via --tls-grpc-listener.", | ||
| help="Require this passphrase from clients connecting via --tls-grpc-listener. " | ||
| "If not provided, a random passphrase is generated automatically.", |
There was a problem hiding this comment.
[LOW] The --passphrase Click option has no envvar parameter, so users must pass secrets as CLI arguments (visible via /proc/<pid>/cmdline). The project already defines JMP_GRPC_PASSPHRASE in jumpstarter/config/env.py for subprocesses, but the CLI entry point does not read from it. Adding envvar="JMP_GRPC_PASSPHRASE" to the Click option definition would let users avoid exposing secrets on the command line.
AI-generated, human reviewed
There was a problem hiding this comment.
Good point. Adding envvar="JMP_GRPC_PASSPHRASE" to the Click option is a small change. Will include it in this round of fixes.
There was a problem hiding this comment.
Fixed in 7a6a596. Added envvar="JMP_GRPC_PASSPHRASE" to the --passphrase Click option definition.
| with tempfile.NamedTemporaryFile(suffix=".pem", delete=False) as cert, \ | ||
| tempfile.NamedTemporaryFile(suffix=".pem", delete=False) as key: | ||
| cert.write(b"dummy cert") | ||
| key.write(b"dummy key") | ||
| cert_path = cert.name | ||
| key_path = key.name | ||
|
|
||
| try: | ||
| result = self._invoke([ | ||
| "--exporter-config", "/dev/null", | ||
| "--tls-grpc-listener", "1234", | ||
| "--tls-cert", cert_path, | ||
| "--tls-key", key_path, | ||
| "--unsafe-no-auth", | ||
| ]) | ||
| assert result.exit_code == 0 | ||
| assert "running without authentication" in result.stderr | ||
| assert "completely unprotected" not in result.stderr | ||
| finally: | ||
| os.unlink(cert_path) | ||
| os.unlink(key_path) |
There was a problem hiding this comment.
[LOW] test_unsafe_no_auth_without_tls_insecure_warns_no_auth creates NamedTemporaryFile objects with delete=False and manually calls os.unlink in a finally block. If the process is killed between creation and cleanup, temp files leak. Consider using pytest's tmp_path fixture for automatic cleanup.
AI-generated, human reviewed
There was a problem hiding this comment.
Agreed. Will switch to pytest's tmp_path fixture for automatic cleanup.
There was a problem hiding this comment.
Fixed in 7a6a596. Switched to pytest's tmp_path fixture for automatic cleanup.
| if passphrase and unsafe_no_auth: | ||
| raise click.UsageError("--passphrase and --unsafe-no-auth are mutually exclusive") | ||
|
|
||
| # Auto-generate a passphrase when none is provided and auth is not explicitly disabled | ||
| if not passphrase and not unsafe_no_auth: |
There was a problem hiding this comment.
[LOW] The passphrase validation at lines 259 and 263 relies on Python truthiness (if passphrase and ..., if not passphrase and ...), which conflates None (not provided) with "" (empty string). This happens to match the intended behavior, but the contract is implicit. Adding passphrase = passphrase or None at the top of the function would make the "empty string equals no passphrase" rule explicit.
AI-generated, human reviewed
There was a problem hiding this comment.
Agreed. Will add passphrase = passphrase or None at the top of _validate_standalone_auth to make the contract explicit.
There was a problem hiding this comment.
Fixed in 7a6a596. Added passphrase = passphrase or None at the top of the function to make the contract explicit.
Fix plan for review round 5Addressing 11 of 12 review comments from @raballew. Pushing back on 1 (regex for passphrase extraction in Go test -- the current Changes to
|
- Fix CliRunner to use mix_stderr=False so result.stderr works correctly - Remove dead `if tls_insecure: pass` branch in _handle_child - Add type annotations to _validate_standalone_auth - Add `passphrase = passphrase or None` for explicit truthiness handling - Remove docstring and inline narration comments per project convention - Add envvar="JMP_GRPC_PASSPHRASE" to --passphrase Click option - Switch _serve_with_exc_handling call to keyword arguments - Update passphrase forwarding assertion to use call_args.kwargs - Add direct unit tests for _validate_standalone_auth branches - Switch to tmp_path fixture for automatic cleanup Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Review feedback addressed (round 5)Addressed 11 of 12 review comments from @raballew (commit 7a6a596): [HIGH] CliRunner mix_stderr -- Fixed. Both [MEDIUM] Dead [MEDIUM] Unnecessary docstring and comments -- Fixed. Removed docstring from [MEDIUM] Type annotations -- Fixed. Added [MEDIUM] Fragile [MEDIUM] Passphrase extraction regex -- Pushed back. The [LOW] Direct unit tests for [LOW] [LOW] [LOW] [LOW] |
CI fix planThe In Click 8.2+, stderr is captured separately by default. The fix is to remove |
Click 8.2+ removed the mix_stderr parameter from CliRunner (stderr is separated by default). The installed Click 8.3.1 raises TypeError when mix_stderr is passed, causing all 7 auth validation tests to fail. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
CI fix applied (commit 8b2c56d)Removed This fixes all 7 failing |
Status UpdateAll review feedback from round 5 has been addressed and all CI checks are passing: Review feedback addressed (commit 8b2c56d):
CI Status: All checks passing (28 passed, 4 skipped)
The PR is ready for re-review. @raballew could you please take another look when you have a chance? |
2 participants
Summary
Fixes #356 — standalone mode previously exposed an unauthenticated, unencrypted gRPC server by default.
secrets.token_urlsafe(32)) when--passphraseis not provided in standalone mode, printed to stderr for sharing with clients--unsafe-no-authflag to explicitly disable authentication (mutually exclusive with--passphrase)--unsafe-no-authis used, with a stronger warning when combined with--tls-grpc-insecureTest plan
make lint-fix)--unsafe-no-auth allows unauthenticated accessauto-generated passphrase is printed to stderr and can be used to connect(includes negative test with wrong passphrase)--passphrase and --unsafe-no-auth are mutually exclusivejmp run --exporter myconfig --tls-grpc-listener 1234 --tls-grpc-insecurenow auto-generates and prints a passphrasejmp run --exporter myconfig --tls-grpc-listener 1234 --tls-grpc-insecure --unsafe-no-authwarns but allows unauthenticated access--passphraseand--unsafe-no-authtogether produces a UsageError🤖 Generated with Claude Code