Skip to content

Commit e8110c2

Browse files
committed
refactor: converge transport, exit-code and masking seams, mask @export by default
Behavior changes (consumers must act): - `@export <VAR>[,<VAR>] [--public]`: a captured value now joins the mask registry for every render path (terminal, `-v` preview, `--log-file`). The old rule decided whether to mask by whether the *variable name* looked secret, so a credential exported as `DATABASE_URL` landed in the audit trail in the clear while an `@env` literal of the same value was masked. `--public` is the explicit, reviewable opt-out for a value that is meant to be read (`VERSION`, `STAMP`, a service name). - A step that exceeds `--timeout`/`@timeout` now exits 5 instead of being classified as 4 (script failure), so a CI retry policy can tell "the deploy broke" from "the box was slow". `bin/shellflow/src/exit.rs` is the single definition of both protocols (the CLI's and the target-side helper's, which is a wire contract with the generated payloads). Architecture: - `ssh` options (`BatchMode`, `-T`, the connect bound, `ControlMaster`) are decided once in `executor::transport`; all eight argv builders now go through it. `--no-ssh-reuse` went from covering 1 of 8 call sites to all of them, and two sites that had silently lost `-T` got it back. `SshSpec::rsync_transport` is deleted: a transport policy string no longer lives in the pure grammar crate. - The login-shell probe is a per-destination single flight instead of a global lock held across a network round trip, so `--parallel N` no longer degrades to serial probing (20 hosts: 250s worst case -> ~12.5s). - `RunError`/`RunOutcome` moved to `executor/error.rs` (the crate-level taxonomy no longer lives in the largest leaf module and no longer flows backwards into its siblings), and the systemd helper protocol moved to `executor/helper.rs`: the exit-code ladder that was spelled out four times is now one `classify_helper_exit`. - `ExecutionPlan` fields are private behind accessors, public enums are `#[non_exhaustive]`, and the `Step`/`Assertion` matches that were duplicated five to six times collapse into `Step::target()` plus honest fallback arms. - `rbatch` lost its synchronous/async twins; both now share one implementation behind `spawn_blocking`. - Masking moved down into the sink (`print_line`, `Ui::emit_host_line`), so a call site that forgets to redact can no longer resurrect the leak; the two `eprintln!` setup failures and every subcommand error report now go through `ui`. - The `--no-default-features` build compiles again: `StepClass` is exported for the ssh-only configuration, where it is a parameter of the step dispatcher. Security: - The age identity check now verifies the owner as well as the mode bits and opens with `O_NOFOLLOW`, so a 0400 file owned by another user or a symlink planted at the identity path is refused. - `fsx` fsyncs the parent directory after `rename`/`hard_link`, closing the window where a crash could drop a file the tool reported as written. - `template render` keeps its rendered document in a `Zeroizing` buffer. Docs: docs/design.md (4.1, 5.1, 5.2, 6, 7.6, 9, S5.3), README.md and SKILL.md now match the code.
1 parent b15c20c commit e8110c2

38 files changed

Lines changed: 1824 additions & 940 deletions

‎README.md‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ target-side daemon.
1919
# @local
2020
VERSION=$(git rev-parse --short HEAD)
2121

22-
# @export VERSION
22+
# @export VERSION --public
2323

2424
# @copy target/release/myapp-$VERSION -> /tmp/shellflow/myapp-$VERSION @all
2525

@@ -64,7 +64,10 @@ deploy.sh` runs it unchanged, and your editor highlights it perfectly.
6464
warning instead of being performed: a dry run never writes to a target.
6565
- **State passing** — `@export VAR` captures a variable from a local block and
6666
injects it into later `@copy` paths, `@remote` blocks, and `@only_if` guards
67-
as `$VAR` / `${VAR}` / env.
67+
as `$VAR` / `${VAR}` / env. A captured value is **masked in all output** by
68+
default (it is a value the tool did not see at parse time, and a playbook
69+
routinely exports a credential it just fetched); add `--public` for a value
70+
that is meant to be read, such as a version or a commit.
6871
- **Graceful file copies** — `@copy` uses rsync when the target has it (with a
6972
`mkdir -p` remote wrapper), and automatically falls back to
7073
`ssh mkdir -p` + `scp` on hosts without rsync. Destination directories are
@@ -142,7 +145,7 @@ lines belong to the current block.
142145
| `@local` | `# @local` | Following lines run locally (default) |
143146
| `@remote` | `# @remote <target>` | Following lines stream to the target (alias, group, or raw spec) |
144147
| `@copy` | `# @copy <src> -> <dst> @<target> [--delete] [--exclude <glob>]` | Copy a local path to the target (rsync, or scp fallback); a directory source is flattened into `dst` (contents semantics, identical on GNU rsync and macOS openrsync); supports `$VAR` interpolation; `--exclude` is repeatable and honored by rsync (warned as ignored on the scp fallback and in `--local` mode) |
145-
| `@export` | `# @export <VAR>[,<VAR>…]` | Capture variables from the preceding local block into run state |
148+
| `@export` | `# @export <VAR>[,<VAR>…] [--public]` | Capture variables from the preceding local block into run state. Captured values are masked in all output by default; `--public` opts out for a value that is meant to be read |
146149
| `@timeout` | `# @timeout <seconds>` | Per-step timeout for the next block |
147150
| `@name` | `# @name <label>` | Name the next block (for `--only`/`--skip` and reporting) |
148151
| `@only_if` | `# @only_if <command>` | Skip the next block where `<command>` fails |

‎SKILL.md‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ lines belong to the current block.
7070
| `@local` | `# @local` | Following lines run locally (default) |
7171
| `@remote` | `# @remote <target>` | Following lines stream to the target (alias, group, raw spec) |
7272
| `@copy` | `# @copy <src> -> <dst> @<target> [--delete] [--exclude <glob>]` | Copy a local path (rsync, or scp fallback); creates the destination dir; `$VAR` interpolated; `--exclude` is repeatable and honored by rsync. On a host without rsync the scp fallback cannot filter or mirror, so a step that asked for `--delete`/`--exclude` **fails closed** unless `--allow-unfiltered-copy` accepts the downgrade |
73-
| `@export` | `# @export <VAR>[,<VAR>…]` | Capture variables from the preceding local block into run state |
73+
| `@export` | `# @export <VAR>[,<VAR>…] [--public]` | Capture variables from the preceding local block into run state. Every captured value is masked in the output (terminal, previews, `--log-file`) unless the line says `--public`; use `--public` for versions, timestamps, paths and other non-secret values |
7474
| `@timeout` | `# @timeout <seconds>` | Per-step timeout for the next block |
7575
| `@name` | `# @name <label>` | Name the next block (for `--only`/`--skip`/reporting) |
7676
| `@only_if` | `# @only_if <command>` | Skip the next block where `<command>` fails |
@@ -112,6 +112,9 @@ Behavioral rules to remember:
112112
- `@copy` paths are parsed by shellflow, not Bash. Capture values with
113113
`@export` and reference them as `$VAR` / `${VAR}`. Unresolved variables are
114114
a hard error in a real run, and a warning + skip in `--dry-run`/`--check`.
115+
A captured value is masked in every rendered line unless the declaration says
116+
`--public`; a secret exported under an ordinary name (`DATABASE_URL`) is
117+
therefore still not printed.
115118
- `@only_if` guards run per host with the same env as the block; a failed
116119
guard reports `SKIPPED`.
117120
- `@copy` `src`/`dst` must not contain `->`; the target is the last
@@ -141,7 +144,7 @@ mutations confined to `/tmp` unless the user explicitly asks otherwise.
141144
STAMP="sf-$(date +%s)"
142145
WORKDIR="/tmp/${STAMP}"
143146
mkdir -p "${WORKDIR}"
144-
# @export STAMP,WORKDIR
147+
# @export STAMP,WORKDIR --public
145148

146149
# @copy ./playbooks/marker.txt -> $WORKDIR/marker.txt @all
147150

‎bin/shellflow/src/executor/assert_step.rs‎

Lines changed: 15 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,9 @@ use tokio::sync::Semaphore;
2323

2424
use super::stream;
2525
use crate::{
26-
executor::{ArcUi, HostOutcome, LogFile, RunError, StepConfig, cancel::Cancel, fanout_slot},
26+
executor::{
27+
ArcUi, HostOutcome, LogFile, RunError, StepConfig, cancel::Cancel, fanout_slot, quote,
28+
},
2729
rbatch::{Batch, SshDest},
2830
};
2931

@@ -42,15 +44,10 @@ fn check_budget(check: &Assertion, step_timeout: Option<u64>) -> u64 {
4244
match check {
4345
Assertion::Port { .. } => step_timeout.unwrap_or(DEFAULT_PORT_TIMEOUT_SECS),
4446
Assertion::Health { .. } => step_timeout.unwrap_or(DEFAULT_HEALTH_TIMEOUT_SECS),
45-
// The unit-state, enablement, port-free, and rendered checks answer
46-
// at once.
47-
Assertion::UnitState { .. } |
48-
Assertion::Enabled { .. } |
49-
Assertion::PortFree { .. } |
50-
Assertion::Rendered { .. } |
51-
Assertion::NoLeak { .. } |
52-
Assertion::Digest { .. } |
53-
Assertion::Credstore { .. } => 0,
47+
// The unit-state, enablement, port-free, rendered, leak, digest and
48+
// credential-store checks all answer at once — and so would a variant
49+
// this build does not know (`Assertion` is `#[non_exhaustive]`).
50+
_ => 0,
5451
}
5552
}
5653

@@ -393,6 +390,11 @@ fn command_for(check: &Assertion, step_timeout: Option<u64>) -> String {
393390
helper = quote(crate::executor::host::HELPER_PATH),
394391
service = quote(service),
395392
),
393+
// `Assertion` is `#[non_exhaustive]`: an unknown check cannot be
394+
// evaluated, and reporting it as passed would be a lie — the command
395+
// fails and says why.
396+
_ => "echo 'this build does not recognise the assertion (upgrade shellflow)'; false"
397+
.to_string(),
396398
}
397399
}
398400

@@ -442,14 +444,9 @@ fn status_glob(pattern: &str) -> String {
442444
pattern.to_string()
443445
}
444446

445-
/// Quote a value for the remote shell.
446-
///
447-
/// The value comes from the playbook (a unit name, a state, a URL), so it
448-
/// is quoted rather than trusted: a unit name with a space must not become
449-
/// two arguments.
450-
fn quote(value: &str) -> String {
451-
shellflow_core::shell_quote_single(value)
452-
}
447+
// `quote` is the executor-wide helper (`super::quote`): the value comes from
448+
// the playbook (a unit name, a state, a URL), so it is quoted rather than
449+
// trusted — a unit name with a space must not become two arguments.
453450

454451
#[cfg(test)]
455452
mod tests {

‎bin/shellflow/src/executor/build.rs‎

Lines changed: 35 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
//! runner, so a new service adds one declarative block instead of a new
88
//! build script.
99
10-
use super::host::quote;
10+
use crate::executor::quote;
1111

1212
/// The remote command for `@build`.
1313
///
@@ -161,22 +161,27 @@ pub(super) fn build_command(build: &shellflow_core::BuildStep) -> String {
161161
mod tests {
162162
use super::build_command;
163163

164+
/// Build a `BuildStep` through the parser.
165+
///
166+
/// The payload structs are `#[non_exhaustive]`, so the DSL is the only
167+
/// constructor a consumer has — which is the point: a step carries
168+
/// invariants the parser enforced (a declared target, a required
169+
/// `--build`), and a struct literal would skip them.
170+
fn step(directive: &str) -> shellflow_core::BuildStep {
171+
let script = format!("# @server api api\n{directive}\n");
172+
let plan = shellflow_core::parse_script(&script).expect("the fixture parses");
173+
match &plan.steps()[0] {
174+
shellflow_core::Step::Build(step) => step.clone(),
175+
other => panic!("expected a build step, got {other:?}"),
176+
}
177+
}
178+
164179
fn sample() -> String {
165-
build_command(&shellflow_core::BuildStep {
166-
name: None,
167-
target: shellflow_core::Target::new("api"),
168-
repo: "https://github.com/x/y.git".to_string(),
169-
git_ref: "abc123".to_string(),
170-
build_cmd: "cargo build --release -p app".to_string(),
171-
prep_cmd: None,
172-
workdir: None,
173-
artifacts: vec!["target/release/app".to_string()],
174-
install_dirs: vec!["/opt/app/bin".to_string()],
175-
units: vec!["app.service".to_string()],
176-
version_flag: "--version".to_string(),
177-
min_free_gb: 8,
178-
timeout: None,
179-
})
180+
build_command(&step(
181+
"# @build https://github.com/x/y.git @api --ref abc123 \
182+
--build 'cargo build --release -p app' --artifacts target/release/app \
183+
--install-dirs /opt/app/bin --units app.service --version-flag=--version",
184+
))
180185
}
181186

182187
#[test]
@@ -209,41 +214,19 @@ mod tests {
209214
let cmd = sample();
210215
assert!(cmd.contains("--version"), "{cmd}");
211216
// …and falls back to the flag chain when no flag is declared.
212-
let noflag = build_command(&shellflow_core::BuildStep {
213-
name: None,
214-
target: shellflow_core::Target::new("api"),
215-
repo: "https://github.com/x/y.git".to_string(),
216-
git_ref: "r".to_string(),
217-
build_cmd: "make".to_string(),
218-
prep_cmd: None,
219-
workdir: None,
220-
artifacts: vec!["app".to_string()],
221-
install_dirs: vec!["/opt/a".to_string()],
222-
units: vec![],
223-
version_flag: String::new(),
224-
min_free_gb: 8,
225-
timeout: None,
226-
});
217+
let noflag = build_command(&step(
218+
"# @build https://github.com/x/y.git @api --ref r --build make \
219+
--artifacts app --install-dirs /opt/a",
220+
));
227221
assert!(noflag.contains("--version -V -v -version version"), "{noflag}");
228222
}
229223

230224
#[test]
231225
fn build_values_are_quoted() {
232-
let cmd = build_command(&shellflow_core::BuildStep {
233-
name: None,
234-
target: shellflow_core::Target::new("api"),
235-
repo: "https://github.com/x/y.git".to_string(),
236-
git_ref: "r; evil".to_string(),
237-
build_cmd: "make".to_string(),
238-
prep_cmd: None,
239-
workdir: None,
240-
artifacts: vec!["a".to_string()],
241-
install_dirs: vec!["/opt/a".to_string()],
242-
units: vec![],
243-
version_flag: String::new(),
244-
min_free_gb: 8,
245-
timeout: None,
246-
});
226+
let cmd = build_command(&step(
227+
"# @build https://github.com/x/y.git @api --ref 'r; evil' --build make \
228+
--artifacts a --install-dirs /opt/a",
229+
));
247230
assert!(cmd.contains("'r; evil'"), "{cmd}");
248231
}
249232

@@ -255,21 +238,12 @@ mod tests {
255238

256239
#[test]
257240
fn build_installs_renamed_artifacts_under_the_new_name() {
258-
let mut step = shellflow_core::BuildStep {
259-
name: None,
260-
target: shellflow_core::Target::new("ai"),
261-
repo: "https://github.com/bnb-chain/reth.git".to_string(),
262-
git_ref: "5710c1f".to_string(),
263-
build_cmd: "cargo build --release -p reth".to_string(),
264-
prep_cmd: None,
265-
workdir: None,
266-
artifacts: vec!["target/release/reth=reth-bsc".to_string()],
267-
install_dirs: vec!["/usr/local/bin".to_string()],
268-
units: vec!["bsc-reth-testnet.service".to_string()],
269-
version_flag: "--version".to_string(),
270-
min_free_gb: 20,
271-
timeout: None,
272-
};
241+
let mut step = step(
242+
"# @build https://github.com/bnb-chain/reth.git @api --ref 5710c1f \
243+
--build 'cargo build --release -p reth' \
244+
--artifacts target/release/reth=reth-bsc --install-dirs /usr/local/bin \
245+
--units bsc-reth-testnet.service --version-flag=--version --min-free-gb 20",
246+
);
273247
let cmd = build_command(&step);
274248
// The installed file and its backup carry the *new* name; the
275249
// built file keeps the workspace name.

‎bin/shellflow/src/executor/copy.rs‎

Lines changed: 25 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,9 @@ use shellflow_core::{CopyStep, ResolvedHost};
88
use tokio::{process::Command, task::JoinSet};
99

1010
use super::{
11-
ArcUi, CopyStepArgs, DiffFilter, HostOutcome, LogFile, StepConfig, fanout_slot,
11+
ArcUi, CopyStepArgs, DiffFilter, HostOutcome, LogFile, RunError, StepConfig, fanout_slot,
1212
process::{GroupGuard, isolate},
13-
remote::{RunError, resolve_remote_env},
13+
remote::resolve_remote_env,
1414
stream,
1515
};
1616

@@ -340,7 +340,7 @@ async fn run_copy_step_local(args: LocalCopyArgs<'_>) -> Result<Vec<(String, Hos
340340
// after `cp` succeeded. Wiping first (the previous behaviour) meant a
341341
// failed copy left the destination empty — the worst possible outcome
342342
// for a `--delete` deploy.
343-
return run_local_copy_delete(src, dst, config, cancel).await;
343+
return run_local_copy_delete(src, dst, config, cancel, log_file).await;
344344
}
345345

346346
if let Some(parent) = std::path::Path::new(dst).parent() &&
@@ -354,7 +354,7 @@ async fn run_copy_step_local(args: LocalCopyArgs<'_>) -> Result<Vec<(String, Hos
354354
let mut cmd = Command::new("cp");
355355
cmd.arg("-a").arg(src).arg(dst);
356356
cmd.kill_on_drop(true);
357-
let status = run_command_status(cmd, config.timeout, "local copy", cancel).await?;
357+
let status = run_command_status(cmd, config.timeout, "local copy", cancel, log_file).await?;
358358
if !status.success() {
359359
bail!("local copy exited with status {status}");
360360
}
@@ -372,6 +372,7 @@ async fn run_local_copy_delete(
372372
dst: &str,
373373
config: StepConfig,
374374
cancel: &super::Cancel,
375+
log_file: &LogFile,
375376
) -> Result<Vec<(String, HostOutcome)>> {
376377
let dst_path = std::path::Path::new(dst);
377378
if let Some(parent) = dst_path.parent().filter(|p| !p.as_os_str().is_empty()) {
@@ -392,7 +393,7 @@ async fn run_local_copy_delete(
392393
let mut cmd = Command::new("cp");
393394
cmd.arg("-a").arg(src).arg(&staging);
394395
cmd.kill_on_drop(true);
395-
let status = run_command_status(cmd, config.timeout, "local copy", cancel).await?;
396+
let status = run_command_status(cmd, config.timeout, "local copy", cancel, log_file).await?;
396397
if !status.success() {
397398
// The old destination is untouched; drop the half-built staging tree.
398399
remove_path(&staging).await?;
@@ -430,6 +431,7 @@ async fn run_command_status(
430431
timeout: Option<u64>,
431432
label: &str,
432433
cancel: &super::Cancel,
434+
log_file: &LogFile,
433435
) -> Result<std::process::ExitStatus> {
434436
isolate(&mut cmd);
435437
cmd.kill_on_drop(true);
@@ -447,7 +449,7 @@ async fn run_command_status(
447449
return Err(err).wrap_err(format!("failed to run {label}"));
448450
}
449451
super::process::ChildOutcome::TimedOut => {
450-
crate::ui::warn(&format!("{label} timed out — killed"), &None);
452+
crate::ui::warn(&format!("{label} timed out — killed"), log_file);
451453
super::process::terminate(&mut child).await;
452454
guard.disarm();
453455
bail!("{label} timed out");
@@ -479,8 +481,13 @@ struct RsyncHostArgs<'a> {
479481
async fn run_rsync_host(args: RsyncHostArgs<'_>) -> (String, HostOutcome) {
480482
let RsyncHostArgs { ui, host, srcs, dst, opts, config, has_rsync, log_file, cancel } = args;
481483
// Never a bare `ssh`: a transport without BatchMode can stop at a
482-
// password or host-key prompt in the middle of an unattended deploy.
483-
let remote_shell = host.spec.rsync_transport();
484+
// password or host-key prompt in the middle of an unattended deploy. The
485+
// string comes from the one policy builder, so `--no-ssh-reuse` and the
486+
// connect bound reach the transfer too.
487+
let remote_shell = super::transport::rsync_transport(
488+
super::transport::SshPolicy::step(config.ssh_reuse, false),
489+
&host.spec,
490+
);
484491

485492
if config.verbose >= 2 {
486493
crate::ui::note(
@@ -510,7 +517,7 @@ async fn run_rsync_host(args: RsyncHostArgs<'_>) -> (String, HostOutcome) {
510517
),
511518
log_file,
512519
);
513-
return (host.alias.clone(), HostOutcome::Failed(super::remote::RunError::Config));
520+
return (host.alias.clone(), HostOutcome::Failed(super::RunError::Config));
514521
}
515522
crate::ui::warn(
516523
&format!(
@@ -615,8 +622,9 @@ async fn run_rsync_transfer(args: RsyncTransferArgs<'_>) -> std::result::Result<
615622
_ => dst.as_str(),
616623
}
617624
};
618-
let quoted = dir.replace('\'', "'\\''");
619-
let mkdir_rsync = format!("mkdir -p '{quoted}' && rsync");
625+
// The remote shell parses `--rsync-path` verbatim, so the directory goes
626+
// through the one quoting helper instead of a hand-rolled `'\''` swap.
627+
let mkdir_rsync = format!("mkdir -p -- {} && rsync", super::quote(dir));
620628
cmd.args(["--rsync-path", &mkdir_rsync, "-e", args.remote_shell]);
621629
let dest = format!("{}:{dst}", args.host.spec.dest());
622630
// Several sources land in one destination directory; rsync keeps each
@@ -727,12 +735,11 @@ async fn run_scp_transfer(
727735
let scp_srcs: Vec<String> =
728736
if srcs.len() == 1 && is_dir_src { vec![format!("{first}/.")] } else { srcs.to_vec() };
729737

730-
let quoted_dir = dir.replace('\'', "'\\''");
731-
let mut mkdir = Command::new("ssh");
732-
// Fail closed on an unknown host key or a missing key: never prompt.
733-
mkdir.args(["-o", "BatchMode=yes"]);
734-
mkdir.args(host.spec.to_ssh_args()).arg("-T");
735-
mkdir.arg(format!("mkdir -p '{quoted_dir}'"));
738+
let mut mkdir = Command::from(super::transport::ssh_command(
739+
super::transport::SshPolicy::step(config.ssh_reuse, false),
740+
&host.spec,
741+
));
742+
mkdir.arg(format!("mkdir -p -- {}", super::quote(&dir)));
736743
mkdir
737744
.stdin(std::process::Stdio::null())
738745
.stdout(std::process::Stdio::null())
@@ -744,7 +751,7 @@ async fn run_scp_transfer(
744751
// Routed through `run_command_status` so the mkdir honours the per-step
745752
// timeout and owns a `GroupGuard`: an unbounded `ssh` here used to hang a
746753
// `--timeout`-bounded copy step forever.
747-
let status = run_command_status(mkdir, mkdir_timeout, "remote mkdir", &cancel)
754+
let status = run_command_status(mkdir, mkdir_timeout, "remote mkdir", &cancel, log_file)
748755
.await
749756
.map_err(|err| CopyFailure::transport(format!("failed to create remote dir: {err:#}")))?;
750757
if !status.success() {

0 commit comments

Comments
 (0)