feat(cargo-wdk): add --signtool-args passthrough to customize driver signing - #699
feat(cargo-wdk): add --signtool-args passthrough to customize driver signing#699Shravan Vasista (svasista-ms) wants to merge 45 commits into
--signtool-args passthrough to customize driver signing#699Conversation
There was a problem hiding this comment.
Pull request overview
Adds a cargo wdk build --signtool-args passthrough so driver signing can be customized (certificate selection, digest, timestamping, extra operands), and adjusts packaging to stage artifacts in a fresh directory and assemble the final package folder last—preventing stale signing artifacts from persisting across rebuilds.
Changes:
- Add
--signtool-argsto the CLI and plumb it through tosigntool signinvocation. - Rework packaging to build in a clean per-build staging directory and rename into the final package folder at the end (fixes stale cert artifact scenarios).
- Expand integration/unit tests and documentation to cover the new signing behavior and staging semantics.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/cargo-wdk/tests/build_command_test.rs | Adds regression + functional integration tests for staging behavior and --signtool-args. |
| crates/cargo-wdk/src/providers/mod.rs | Extends filesystem error enum to cover directory removal failures. |
| crates/cargo-wdk/src/providers/fs.rs | Adds remove_dir_all wrapper to the FS provider for testable directory cleanup. |
| crates/cargo-wdk/src/cli.rs | Introduces --signtool-args and validates signing flag combinations via TryFrom<&BuildArgs> for SignMode. |
| crates/cargo-wdk/src/actions/build/tests.rs | Updates build action unit test expectations for staging-dir + final assembly flow and new SignMode shape. |
| crates/cargo-wdk/src/actions/build/package_task.rs | Implements staging directory flow, package folder assembly, signtool argument tokenization + passthrough, and updates signing behavior. |
| crates/cargo-wdk/src/actions/build/mod.rs | Adjusts BuildAction to clone SignMode (now contains owned data). |
| crates/cargo-wdk/README.md | Documents --signtool-args, quoting/tokenization rules, and updated signing/staging semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #699 +/- ##
==========================================
+ Coverage 80.47% 81.66% +1.19%
==========================================
Files 26 25 -1
Lines 5720 6108 +388
Branches 5720 6108 +388
==========================================
+ Hits 4603 4988 +385
Misses 989 989
- Partials 128 131 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| .expect("args parse"); | ||
| let err = SignMode::try_from(&args).expect_err("should be rejected"); | ||
| assert!( | ||
| err.to_string().contains("`--sign-mode=off`"), |
There was a problem hiding this comment.
- use the exact message for assertion
`--signtool-args` cannot be used with `--sign-mode=off`.
- Can we not match exactly and avoid using contains?
There was a problem hiding this comment.
Fixed ✅, using the full message for assertion 👍
| let err = parse_build_args(&["--signtool-args", "/n \"CN=Contoso"]) | ||
| .expect_err("unterminated quote should be rejected"); | ||
| assert!( | ||
| err.to_string().contains("unterminated"), |
There was a problem hiding this comment.
Try and use exact message assertion
Similar suggestion as this: https://github.com/microsoft/windows-drivers-rs/pull/699/changes#r3579259207
There was a problem hiding this comment.
Fixed ✅
There was a problem hiding this comment.
🟡 Not ready to approve
The CLI help text for --signtool-args is currently misleading relative to the implemented “replace defaults / skip auto-cert” behavior, and should be clarified before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (3)
crates/cargo-wdk/src/cli.rs:251
--signtool-argscurrently accepts a value that includes thesignverb (or other non-option tokens), which then gets prepended with anothersigninrun_signtool_sign, producing a confusing signtool error. Since the README/PR description state that cargo-wdk supplies the verb and file operand, it would be better to reject values starting withsign(case-insensitive) at parse/validation time with a clear clap error.
if in_arg && !current.is_empty() {
args.push(current);
}
Ok(SigntoolArgs(args))
crates/cargo-wdk/src/cli.rs:142
- The
--signtool-argshelp text says “Additional arguments…”, but inPackageTask::sign_and_verifya non-empty value replaces the default test-certificate switches and skips auto cert generation. That difference can mislead users into thinking args are appended to defaults; please clarify the replacement semantics and the fact thatsign/file operand are added by cargo-wdk.
/// Additional arguments to pass to `signtool sign` when signing the driver
/// binary and the catalog file, e.g.
/// `--signtool-args '/fd SHA512 /n "CN=WDRLocalTestCert, O=Foo"'`.
crates/cargo-wdk/src/actions/build/package_task.rs:39
- The default timestamp URL is plain HTTP. Using HTTPS (or switching to signtool’s
/trRFC3161 timestamping) would avoid MITM risk and environments that block non-TLS outbound traffic; consider updating the default to an HTTPS endpoint.
const DEFAULT_TIMESTAMP_URL: &str = "http://timestamp.digicert.com";
- Files reviewed: 13/13 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Not ready to approve
There are concrete CLI/signing UX and security concerns in the updated argument parsing and timestamp URL handling that should be resolved before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (3)
crates/cargo-wdk/src/cli.rs:233
parse_passthrough_argscurrently drops empty quoted tokens (because it only pushes non-empty tokens). That means callers cannot intentionally pass an empty argument value (e.g.,--signtool-args '/p ""'), which breaks the promise of a true signtool passthrough and can also shift subsequent arguments in ways that change meaning.
None if c == '"' || c == '\'' => {
quote = Some(c);
in_arg = true;
}
None if c.is_whitespace() => {
if in_arg {
let token = std::mem::take(&mut current);
if !token.is_empty() {
args.push(token);
}
in_arg = false;
}
crates/cargo-wdk/src/actions/build/package_task.rs:39
DEFAULT_TIMESTAMP_URLuses anhttp://timestamp server URL. Since this is sent over the network during signing, preferhttps://to avoid downgrade/MITM risks and to match modern security expectations (DigiCert supports HTTPS).
const DEFAULT_TIMESTAMP_URL: &str = "http://timestamp.digicert.com";
crates/cargo-wdk/src/cli.rs:172
BuildArgs::sign_modenow constructs aclap::Error(ArgumentConflict), butCli::runreturnsanyhow::Result<()>andmainunconditionally exits withExitCode::FAILUREon any error. This likely discards clap’s intended exit status (typically 2 for usage/argument errors) and may not render the error as clap would (usage/context), so the "consistent CLI UX" goal may not actually be achieved end-to-end.
/// Resolves a typed, fully-validated [`SignMode`] from the parsed build
/// arguments. Rules that clap cannot express declaratively are enforced
/// here and surfaced as `clap::Error` for consistent CLI UX.
fn sign_mode(&self) -> Result<SignMode, clap::Error> {
fn build_error(message: impl std::fmt::Display) -> clap::Error {
Cli::command().error(ErrorKind::ArgumentConflict, message)
}
- Files reviewed: 13/13 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Changes recommended
There’s an unaddressed behavior/UX gap around default signing options (notably timestamping) and documentation clarity that should be resolved before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
crates/cargo-wdk/README.md:126
- The docs imply that any use of
--signtool-argsswitches cargo-wdk into full passthrough mode, but the implementation treats an empty/whitespace value as “no args” (i.e., it falls back to the default WDR test-cert signing). Documenting this explicitly helps avoid surprises when users pass an env var that expands to an empty string.
- When `--signtool-args` is **omitted**, cargo-wdk signs with the auto-generated WDR test certificate as described above.
- When `--signtool-args` is **provided**, you own the full `signtool sign` option set (certificate selection, digest algorithm, etc.). `cargo-wdk` will prepend the `sign` verb to your arguments and append the trailing file operand so you should not provide them.
`--signtool-args` applies only when signing is enabled; supplying it with `--sign-mode=off` is an error.
crates/cargo-wdk/src/actions/build/package_task.rs:308
- The default (non-passthrough)
signtool signargument list no longer includes any timestamping option (e.g./tor/tr). Without timestamping, signatures can become invalid after the signing certificate expires, which is a potentially significant behavior change for produced artifacts. Consider either restoring a default timestamp switch (as previously used) or explicitly documenting that timestamping is left to the caller via--signtool-args.
WDR_TEST_CERT_STORE,
"/n",
WDR_LOCAL_TEST_CERT,
"/fd",
"SHA256",
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
crates/cargo-wdk/tests/build_command_test.rs:588
- This assertion still uses the previous generic error substring ("signing driver binary"), but the new error path reports failures as "Error signing using signtool". Using the updated substring will keep the test aligned with current error formatting while still allowing signtool-version differences.
assert!(
stderr.contains("No file digest algorithm specified")
|| stderr.contains("signing driver binary"),
"expected a signtool failure from the duplicate `sign` verb, got: {stderr}"
);
crates/cargo-wdk/README.md:126
- Docs say that when
--signtool-argsis provided, the caller owns the fullsigntool signoption set. However, the CLI parser treats empty/whitespace values as "no args" and the build path falls back to the default test-cert switches when the parsed args are empty. Consider documenting that empty/whitespace values are treated the same as omitting the flag to avoid surprising behavior.
To sign with your own certificate or tweak any signing option, pass `--signtool-args` with a string of the arguments to forward to `signtool sign`.
- When `--signtool-args` is **omitted**, cargo-wdk signs with the auto-generated WDR test certificate as described above.
- When `--signtool-args` is **provided**, you own the full `signtool sign` option set (certificate selection, digest algorithm, etc.). `cargo-wdk` will prepend the `sign` verb to your arguments and append the trailing file operand so you should not provide them.
`--signtool-args` applies only when signing is enabled; supplying it with `--sign-mode=off` is an error.
| assert!( | ||
| stderr.contains("No certificates were found") | ||
| || stderr.contains("signing driver binary"), | ||
| "expected a signtool certificate-selection failure, got: {stderr}" | ||
| ); |
There was a problem hiding this comment.
Why is this test passing if the text we check is wrong?
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (1)
crates/cargo-wdk/src/actions/build/package_task.rs:39
- The default timestamp URL is plain HTTP. That allows MITM tampering with timestamp responses and undermines the integrity guarantees of Authenticode timestamping. Prefer HTTPS for the default TSA endpoint, and update the associated unit tests that hardcode the URL in expected signtool args.
const DEFAULT_TIMESTAMP_URL: &str = "http://timestamp.digicert.com";
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (1)
crates/cargo-wdk/src/cli.rs:195
- The PR description says that when
--signtool-argsis provided, the caller owns the fullsigntool signoption set. However,--signtool-args ''/ whitespace currently parses to an empty vector and is treated the same as omitting the flag (default test cert + default switches). If that’s not intended, reject an explicitly-provided-but-empty value so the behavior matches the documented contract.
SignModeArg::Test => Ok(SignMode::Test {
verify_signature: self.verify_signature,
signtool_args: self
.signtool_args
.clone()
.map(|parsed| parsed.0)
.unwrap_or_default(),
}),
Adds
--signtool-argspassthrough so driver signing can be customized (certificate selection, digest algorithm, timestamping, extra file operands).When
--signtool-argsis omitted,cargo-wdksigns with the auto-generated WDR test certificate and default switches.When
--signtool-argsis provided, the caller owns the fullsigntool signoption set except thesignverb and the trailing file operand (spplied bycargo-wdk). Supplying it with--sign-mode=offis rejected.Packaging removes any existing
<target>/<profile>/<name>_packagefolder at the start of each build and recreates it, so stale signing artifacts (e.g. aWDRLocalTestCert.cerfrom a previous--sign-mode=testbuild) don't persist across rebuilds.Screenshots
Resolves #605