feat: consolidate TLS flags into single --insecure option#333
Open
raballew wants to merge 13 commits intojumpstarter-dev:mainfrom
Open
feat: consolidate TLS flags into single --insecure option#333raballew wants to merge 13 commits intojumpstarter-dev:mainfrom
raballew wants to merge 13 commits intojumpstarter-dev:mainfrom
Conversation
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughConsolidates multiple TLS/insecure CLI flags into a single Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as User
participant CLI as CLI (jmp)
participant Auth as AuthServer
participant Config as LocalConfig
rect rgba(200,200,255,0.5)
User->>CLI: run `jmp login --insecure [--nointeractive]`
end
CLI->>CLI: confirm_insecure(insecure, nointeractive)
CLI->>Auth: fetch_auth_config(login_endpoint, insecure)
Auth-->>CLI: auth config (or error)
CLI->>Config: build TLSConfig(insecure)
CLI->>Auth: perform token exchange using TLSConfig
Auth-->>CLI: token / error
CLI->>Config: save credentials
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/packages/jumpstarter-cli/jumpstarter_cli/login.py`:
- Around line 30-33: The code currently only adds a scheme when missing, which
allows an explicit "http://" login_endpoint to bypass the insecure flag; update
the logic in the login function (where login_endpoint and insecure are handled)
to first check for an explicit "http://" prefix and, if present and insecure is
False, fail/require confirmation (i.e., raise or surface the same
warning/confirmation path), otherwise proceed; only if no scheme is present
(login_endpoint does not start with "http://" or "https://") should you
normalize by prepending scheme = "http" if insecure else "https". Ensure you
reference and enforce the insecure flag when
login_endpoint.startswith("http://") so explicit HTTP endpoints cannot bypass
the gate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eafff20c-ca0f-4672-9f80-b788be09119e
📒 Files selected for processing (12)
.gitignoree2e/tests.batspython/docs/source/getting-started/configuration/authentication.mdpython/docs/source/getting-started/guides/setup-distributed-mode.mdpython/packages/jumpstarter-cli-admin/jumpstarter_cli_admin/create.pypython/packages/jumpstarter-cli-admin/jumpstarter_cli_admin/create_test.pypython/packages/jumpstarter-cli-admin/jumpstarter_cli_admin/import_res.pypython/packages/jumpstarter-cli-admin/jumpstarter_cli_admin/import_res_test.pypython/packages/jumpstarter-cli-common/jumpstarter_cli_common/opt.pypython/packages/jumpstarter-cli-common/jumpstarter_cli_common/opt_test.pypython/packages/jumpstarter-cli/jumpstarter_cli/login.pypython/packages/jumpstarter-driver-flashers/README.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/packages/jumpstarter-cli/jumpstarter_cli/login_test.py`:
- Around line 13-17: The test test_fetch_auth_config_allows_http_with_insecure
currently relies on a real network failure; instead, patch
aiohttp.ClientSession.get (or use a local async test server) to make the test
deterministic: mock ClientSession.get to capture the requested URL and ensure it
is called with "http://login.example.com" when invoking
fetch_auth_config("http://login.example.com", insecure=True), then either return
a controlled mock response or raise a specific, expected exception and assert
the call occurred; update the assertion to verify the mocked call was made and
that insecure=True path in fetch_auth_config was exercised rather than catching
any Exception.
In `@python/packages/jumpstarter-cli/jumpstarter_cli/login.py`:
- Line 120: The current confirm_insecure(insecure, nointeractive) call
under-warns the case where a scheme-less login_target will fall back to plain
HTTP; update the login flow so the user sees an explicit HTTP fallback warning:
after parsing login_target into its endpoint (the code that resolves/normalizes
the target), either (A) call a new login-specific prompt (e.g.,
confirm_insecure_login(...)) that mentions "TLS disabled and will use plain HTTP
for this endpoint" or (B) extend confirm_insecure to accept a parameter like
will_use_http (or parsed_endpoint) and include the HTTP-specific text when true;
adjust the call site in login.py to pass the parsed endpoint/flag and ensure
nointeractive behavior is preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bd544a98-24bd-463d-bafd-003f1ccca04e
📒 Files selected for processing (2)
python/packages/jumpstarter-cli/jumpstarter_cli/login.pypython/packages/jumpstarter-cli/jumpstarter_cli/login_test.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/packages/jumpstarter-cli-common/jumpstarter_cli_common/opt.py`:
- Around line 102-105: Update the confirmation prompt in confirm_insecure to
reflect the broader "insecure mode" semantics (not just TLS verification) by
changing the message passed to click.confirm to mention that insecure mode may
disable TLS verification and allow plain HTTP; keep the same conditional flow
and abort behavior in the confirm_insecure function so the prompt text (in
confirm_insecure) is the only change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 69258c9b-eb49-4fd8-9aa6-96bb5ef884ad
📒 Files selected for processing (1)
python/packages/jumpstarter-cli-common/jumpstarter_cli_common/opt.py
Member
|
Insecure could mean a few things: I would suggest the behavior is instead: I think it's better to be explicit about what is insecure, tls in this case. But I would like to hear @bennyz @kirkbrauer @bkhizgiy |
raballew
force-pushed
the
003-fix-tls-flag-naming
branch
from
6775d35 to
8b91d28
Compare
Resolves jumpstarter-dev#318 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…flags Consolidate all TLS/insecure flags into a single --insecure flag. Remove --insecure-login-tls and --insecure-login-http since --insecure covers the entire connection chain including login endpoint. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The e2e login test still used the removed --insecure-login-http flag, causing 5 cascading test failures in CI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Replace generic confirm_insecure with login-specific prompt that warns about both TLS disabled and plain HTTP usage - Mock aiohttp.ClientSession in test to avoid network dependency Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…n login Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
raballew
force-pushed
the
003-fix-tls-flag-naming
branch
from
8b91d28 to
35f6bb3
Compare
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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
2 participants
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
--insecure-tls-config/--insecure-tlsto--insecureacross all CLI commands--insecure-login-tlsand--insecure-login-httpfromjmp login----insecurenow covers the entire connection chain--insecureis passed and no URL scheme is provided for login, HTTP is used; otherwise HTTPS with TLS verification disabledjmp login,jmp-admin create client/exporter,jmp-admin import client/exporterCloses #318
Test plan
--insecureflag accepted and defaults to falsejmp login --helpshows only--insecure, no--insecure-tls-*or--insecure-login-*flagsjmp-admin create client --helpshows--insecurejmp-admin create exporter --helpshows--insecurejmp-admin import client --helpshows--insecurejmp-admin import exporter --helpshows--insecure🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Refactor
New Features
Tests
Chores