fix: collect --password once up-front and honor -S consistently across subcommands#201
Merged
Merged
Conversation
Fixes two defects in credential collection that surfaced together in `bssh -C orin -l lablup --password -S ping` (issue #200). Defect 1 — `--password` is collected lazily per-connection. The previous implementation prompted via `rpassword::prompt_password()` inside each per-node authentication task (`ssh/auth.rs::password_auth()`). The executor fans out N parallel connection attempts, so multiple tasks raced for stdin: the prompt could be missed, repeated per node, or interleaved with the indicatif progress UI rendering. The `-S` (sudo-password) path already did this correctly — collect once in the dispatcher, wrap in `Arc<SudoPassword>`, share across tasks — and now `--password` follows the same pattern. Defect 2 — `-S` was silently dropped by `ping`, `upload`, and `download`. The dispatcher only read `cli.sudo_password` in the `exec` and interactive branches. Users could pass `-S` to `ping`/`upload`/`download` without any error, but the flag had no effect. Changes: - Add `src/security/password.rs` mirroring the `SudoPassword` design: a `Password` wrapper backed by `secrecy::SecretString` (auto-zeroized on drop), `prompt_password()` with a non-host-specific prompt ("Enter SSH password (used for all hosts): "), `get_password_from_env()` reading `BSSH_PASSWORD`, and `get_password(warn_env)` combining both. Wrapped in `Arc<Password>` for sharing across per-node tasks without duplicating secret material. - Hoist `--password` collection to `dispatch_command` in `src/app/dispatcher.rs`. Prompt runs once, before any `ParallelExecutor` is built and before any indicatif `MultiProgress` is initialized, so the terminal is in a clean state when reading the password. The resulting `Arc<Password>` is threaded through `ping_nodes`, `FileTransferParams` (upload/download), `ExecuteCommandParams`, `InteractiveCommand`, and the SSH-mode interactive path. - Extend `AuthContext` in `src/ssh/auth.rs` with a `password: Option<Arc<Password>>` field and a `with_pre_collected_password()` builder. `password_auth()` now consumes the pre-collected value when available; the in-task `rpassword::prompt_password()` call remains only as the fallback for the OpenSSH-style "all key methods failed, prompt for password" path (which has no dispatcher-side collection because it is opportunistic). - Thread `ssh_password: Option<Arc<Password>>` through the executor (`ParallelExecutor::with_ssh_password`), `ExecutionConfig`, `ConnectionConfig`, and the SFTP `*_with_jump_hosts` helpers. Every per-node task clones the `Arc` so all tasks observe the same secret without copying it. - For `-S`: emit `tracing::warn!` in the dispatcher when the flag is passed to a subcommand where it has no effect (`ping`, `upload`, `download`, `list`, `cache-stats`). Chose a warning over a hard reject to match typical CLI UX — existing scripts that happen to pass `-S` to non-applicable subcommands keep working but the user gets visible feedback. `exec` and interactive paths continue to honor `-S` as before. Tests: - 9 unit tests in `src/security/password.rs` mirror the structure of `src/security/sudo.rs`: creation, empty rejection, debug redaction, clone independence, `Arc` sharing, env var handling (empty/valid/unset), and a dispatcher-collection-pattern test that verifies a single `get_password()` call yields an `Arc<Password>` observably shared across three simulated per-node tasks. - 2 new tests in `src/ssh/auth.rs`: `test_auth_context_with_pre_collected_password` checks the builder and Arc sharing semantics; `test_password_auth_uses_pre_collected_password` verifies `password_auth()` returns immediately without prompting when the pre-collected value is set (critical: it never blocks on stdin in non-interactive test environments). - All existing tests (1233 lib tests + integration suites) continue to pass. `cargo build --release`, `cargo clippy --all-targets --all-features -- -D warnings`, and `cargo fmt --check` all pass. Closes #200
314ee84 to
8ee5e8b
Compare
…nd paths Addresses pr-reviewer findings on PR #201: - jump::chain::auth::determine_auth_method now consumes Arc<Password> instead of prompting per-call (issue #200 C1) - ssh::client::SshClient::connect_and_execute_with_host_check accepts Arc<Password> so the download glob-resolution step reuses the dispatcher's single prompt (issue #200 C2) - commands::exec::execute_command_with_forwarding now builds AuthMethod::with_password from the pre-collected secret instead of erroring after the user has already entered a password (issue #200 M1)
`tracing::warn!` lands on stdout under the default tracing subscriber, so the `-S has no effect` warning was being mixed into stdout — breaking `bssh ... ping | grep ...` style pipelines and contradicting the PR description's contract. Switch to `eprintln!` to match the existing pattern used for the `BSSH_PASSWORD` env warning in `src/security/password.rs:168-171`.
Add CHANGELOG entry for #201 (collect --password once up-front, -S warning to stderr). Document the new BSSH_PASSWORD environment variable in the README environment-variables section and in the bssh.1 man page. Update the --password flag description in the man page to describe the single-prompt / Arc-shared behavior introduced by #201.
Member
Author
PR Finalization CompleteTestsAll targeted tests pass. The existing Final test counts:
DocumentationAdded in commit
Lint / Format
Labels
The PR was not merged. Ready for merge review. |
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
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.
Fixes #200.
Problem
Two distinct defects in credential collection both surface in:
Defect 1 —
--passwordwas collected lazily, per-connectionssh/auth.rs::password_auth()calledrpassword::prompt_password()inside each per-node authentication task. The executor fans out N parallel connection attempts, so multiple tasks raced for stdin: the prompt could be missed (drowned by the indicatif progress UI), repeated per node, or interleaved with the progress bar rendering.The
-S(sudo-password) path already did this correctly — it collected once in the dispatcher, wrapped the value inArc<SudoPassword>, and shared it across tasks.--passwordnow follows the same pattern.Defect 2 —
-Swas silently dropped byping,upload,downloadThe dispatcher only read
cli.sudo_passwordin theexecand interactive branches. Users could pass-Stoping/upload/downloadwithout any error, but the flag had no effect.Changes
New
Passwordwrapper —src/security/password.rsMirrors the existing
SudoPassworddesign:secrecy::SecretString(auto-zeroized on drop via thesecrecycrate'sSecretStringmachinery).prompt_password()uses a non-host-specific prompt:"Enter SSH password (used for all hosts): ".get_password_from_env()readsBSSH_PASSWORD(discouraged for production but useful for automation).get_password(warn_env)combines env + interactive prompt.Arc<Password>and shared across parallel tasks without copying the secret.Dispatcher hoists
--passwordcollection —src/app/dispatcher.rsThe prompt now runs once at the top of
dispatch_command, before anyParallelExecutoris built and before any indicatifMultiProgressis initialized. This avoids the terminal-state pitfall where indicatif corrupts the prompt rendering. The resultingArc<Password>is threaded through:ping_nodes(...)— new parameterFileTransferParams(upload + download) — new fieldExecuteCommandParams— new fieldInteractiveCommand— new field-Ssemantics for non-applicable subcommandsThe dispatcher now emits
tracing::warn!("--sudo-password (-S) has no effect for the {subcommand} subcommand and will be ignored")when-Sis passed toping,upload,download,list, orcache-stats. Picked a warning over a hard reject because:ssh,git, etc. tend to warn rather than fail on flags that are harmless-but-inapplicable).-Sto non-applicable subcommands continue to work — they just see a visible warning instead of silent no-op.execand interactive paths continue to honor-Sexactly as before.Threading through the auth machinery
AuthContext(src/ssh/auth.rs) gains apassword: Option<Arc<Password>>field andwith_pre_collected_password()builder.password_auth()now consumes the pre-collected value when available. The in-taskrpassword::prompt_password()call is retained only as the fallback for the OpenSSH-style "all key methods failed → prompt for password" path (which is opportunistic and has no dispatcher-side collection point).ParallelExecutor::with_ssh_password(),ExecutionConfig::ssh_password,ConnectionConfig::ssh_password, and the SFTP*_with_jump_hostshelpers all carry theOption<Arc<Password>>through to the auth layer.Arcso all tasks observe the same secret without duplicating the underlying string.Tests
New:
src/security/password.rs— 9 unit testsMirror
src/security/sudo.rstest structure:test_password_creation,test_password_empty_rejection,test_password_debug_redactiontest_clone_independence,test_arc_sharing_empty,_valid,_not_settest_get_password_dispatcher_collection_pattern— explicitly models the dispatcher-level collection pattern: oneget_password()call, wrapped inArc<Password>, observably shared across three simulated per-node tasks. Asserts all three see the same password andArc::strong_count == 4.New:
src/ssh/auth.rs— 2 unit teststest_auth_context_with_pre_collected_password— verifies the builder andArcsharing semantics.test_password_auth_uses_pre_collected_password— critically, verifiespassword_auth()returns immediately without prompting when the pre-collected value is set (which means it never blocks on stdin in non-interactive test environments).Existing tests
All 1233 library unit tests and the integration test suites continue to pass. Pre-existing failing doctest in
src/server/filter/mod.rsis unrelated to this PR (verified by runningcargo test --doconmain).Verification
Behavior comparison
bssh -C cluster --password exec "uptime"bssh -C cluster --password pingbssh -C cluster --password upload f.txt /tmp/bssh -S pingWARN: --sudo-password (-S) has no effect for the 'ping' subcommand and will be ignoredbssh -S exec "sudo apt update"BSSH_PASSWORD=secret bssh -C cluster --password pingAcceptance criteria
bssh -C <cluster> --password <subcommand>prompts exactly once, before any connection attempt or progress UI output.secrecy::SecretStringinPassword).-Seither applies to applicable subcommands or warns when passed to subcommands where it has no effect.test_get_password_dispatcher_collection_pattern).