-
Notifications
You must be signed in to change notification settings - Fork 124
feat: introduce sign mode for driver packaging, allowing skipping of signing steps #649
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 4 commits
ba4efe9
c3a080e
490f323
a13c7e3
1c1374a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -28,7 +28,10 @@ use windows::{ | |||||
|
|
||||||
| #[double] | ||||||
| use crate::providers::{exec::CommandExec, fs::Fs, wdk_build::WdkBuild}; | ||||||
| use crate::{actions::build::error::PackageTaskError, providers::error::FileError}; | ||||||
| use crate::{ | ||||||
| actions::{SignMode, build::error::PackageTaskError}, | ||||||
| providers::error::FileError, | ||||||
| }; | ||||||
|
|
||||||
| // FIXME: This range is inclusive of 25798. Update with range end after /sample | ||||||
| // flag is added to InfVerif CLI | ||||||
|
|
@@ -44,6 +47,7 @@ pub struct PackageTaskParams<'a> { | |||||
| pub target_dir: &'a Path, | ||||||
| pub target_arch: &'a CpuArchitecture, | ||||||
| pub verify_signature: bool, | ||||||
| pub sign_mode: SignMode, | ||||||
| pub sample_class: bool, | ||||||
| pub driver_model: DriverConfig, | ||||||
| } | ||||||
|
|
@@ -52,6 +56,7 @@ pub struct PackageTaskParams<'a> { | |||||
| pub struct PackageTask<'a> { | ||||||
| package_name: String, | ||||||
| verify_signature: bool, | ||||||
| sign_mode: SignMode, | ||||||
| sample_class: bool, | ||||||
|
|
||||||
| // src paths | ||||||
|
|
@@ -162,6 +167,7 @@ impl<'a> PackageTask<'a> { | |||||
| Self { | ||||||
| package_name, | ||||||
| verify_signature: params.verify_signature, | ||||||
| sign_mode: params.sign_mode, | ||||||
| sample_class: params.sample_class, | ||||||
| src_inx_file_path, | ||||||
| src_driver_binary_file_path, | ||||||
|
|
@@ -236,24 +242,36 @@ impl<'a> PackageTask<'a> { | |||||
| self.copy(&self.src_map_file_path, &self.dest_map_file_path)?; | ||||||
| self.run_stampinf()?; | ||||||
| self.run_inf2cat()?; | ||||||
| self.generate_certificate()?; | ||||||
| self.copy(&self.src_cert_file_path, &self.dest_cert_file_path)?; | ||||||
| self.run_signtool_sign( | ||||||
| &self.dest_driver_binary_path, | ||||||
| WDR_TEST_CERT_STORE, | ||||||
| WDR_LOCAL_TEST_CERT, | ||||||
| )?; | ||||||
| self.run_signtool_sign( | ||||||
| &self.dest_cat_file_path, | ||||||
| WDR_TEST_CERT_STORE, | ||||||
| WDR_LOCAL_TEST_CERT, | ||||||
| )?; | ||||||
| match self.sign_mode { | ||||||
| SignMode::Test => { | ||||||
| self.generate_certificate()?; | ||||||
| self.copy(&self.src_cert_file_path, &self.dest_cert_file_path)?; | ||||||
| self.run_signtool_sign( | ||||||
| &self.dest_driver_binary_path, | ||||||
| WDR_TEST_CERT_STORE, | ||||||
| WDR_LOCAL_TEST_CERT, | ||||||
| )?; | ||||||
| self.run_signtool_sign( | ||||||
| &self.dest_cat_file_path, | ||||||
| WDR_TEST_CERT_STORE, | ||||||
| WDR_LOCAL_TEST_CERT, | ||||||
| )?; | ||||||
| } | ||||||
| SignMode::Off => { | ||||||
| info!("Sign mode is 'Off'; skipping certificate generation and signing"); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
We want to use and promote lower case values of sign mode everywhere since they are easier to type. |
||||||
| } | ||||||
| } | ||||||
| self.run_infverif()?; | ||||||
| // Verify signatures only when --verify-signature flag = true is passed | ||||||
| // and signing was done (sign mode is not 'Off'). | ||||||
| if self.verify_signature { | ||||||
| info!("Verifying signatures for driver binary and cat file using signtool"); | ||||||
| self.run_signtool_verify(&self.dest_driver_binary_path)?; | ||||||
| self.run_signtool_verify(&self.dest_cat_file_path)?; | ||||||
| if matches!(self.sign_mode, SignMode::Off) { | ||||||
| warn!("Skipping signature verification because sign mode is 'Off'"); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| } else { | ||||||
| info!("Verifying signatures for driver binary and cat file using signtool"); | ||||||
| self.run_signtool_verify(&self.dest_driver_binary_path)?; | ||||||
| self.run_signtool_verify(&self.dest_cat_file_path)?; | ||||||
| } | ||||||
| } | ||||||
| Ok(()) | ||||||
| } | ||||||
|
|
@@ -636,6 +654,7 @@ mod tests { | |||||
| driver_model: DriverConfig::Kmdf(KmdfConfig::default()), | ||||||
| sample_class: false, | ||||||
| verify_signature: false, | ||||||
| sign_mode: SignMode::Test, | ||||||
| }; | ||||||
| let dest_root = target_dir.join(format!("{package_name}_package")); | ||||||
|
|
||||||
|
|
@@ -699,6 +718,7 @@ mod tests { | |||||
| driver_model: DriverConfig::Kmdf(KmdfConfig::default()), | ||||||
| sample_class: false, | ||||||
| verify_signature: false, | ||||||
| sign_mode: SignMode::Test, | ||||||
| }; | ||||||
|
|
||||||
| let command_exec = CommandExec::default(); | ||||||
|
|
@@ -725,6 +745,7 @@ mod tests { | |||||
| driver_model: DriverConfig::Kmdf(KmdfConfig::default()), | ||||||
| sample_class: false, | ||||||
| verify_signature: false, | ||||||
| sign_mode: SignMode::Test, | ||||||
| }; | ||||||
|
|
||||||
| let command_exec = CommandExec::default(); | ||||||
|
|
@@ -760,6 +781,7 @@ mod tests { | |||||
| driver_model: DriverConfig::Kmdf(KmdfConfig::default()), | ||||||
| sample_class: false, | ||||||
| verify_signature: false, | ||||||
| sign_mode: SignMode::Test, | ||||||
| }; | ||||||
|
|
||||||
| let wdk_build = WdkBuild::default(); | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,6 +29,7 @@ use crate::providers::{ | |
| use crate::{ | ||
| actions::{ | ||
| Profile, | ||
| SignMode, | ||
| build::{BuildAction, BuildActionParams, error::BuildActionError}, | ||
| to_target_triple, | ||
| }, | ||
|
|
@@ -265,6 +266,132 @@ pub fn given_a_driver_project_when_verify_signature_is_true_then_it_builds_succe | |
| ); | ||
| } | ||
|
|
||
| // Given: A driver project | ||
| // When: --sign-mode=off is provided | ||
| // Then: It builds successfully and skips all certificate generation and | ||
| // signing steps (no makecert / certmgr / signtool sign calls). | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove these comments for all the new tests for consistency because we don't use them on existing tests either. The function names themselves are super verbose so no need for comments. |
||
| #[test] | ||
| pub fn given_a_driver_project_when_sign_mode_is_off_then_signing_and_verification_steps_are_skipped() | ||
| { | ||
| // Input CLI args | ||
| let cwd = PathBuf::from("C:\\tmp"); | ||
| let profile = None; | ||
| let target_arch = CpuArchitecture::Amd64; | ||
| let verify_signature = false; | ||
| let sample_class = false; | ||
|
|
||
| // Driver project data | ||
| let driver_type = "KMDF"; | ||
| let driver_name = "sample-kmdf"; | ||
| let driver_version = "0.0.1"; | ||
| let wdk_metadata = get_cargo_metadata_wdk_metadata(driver_type, 1, 33); | ||
| let (workspace_member, package) = | ||
| get_cargo_metadata_package(&cwd, driver_name, driver_version, Some(&wdk_metadata)); | ||
|
|
||
| let cargo_build_output = | ||
| create_cargo_build_output_json(driver_name, driver_version, &cwd, None, profile); | ||
| let test_build_action = &TestBuildAction::new(cwd.clone(), profile, None, sample_class) | ||
| .with_sign_mode(SignMode::Off) | ||
| .set_up_standalone_driver_project((workspace_member, package)) | ||
| .expect_default_build_task_steps(driver_name, Some(cargo_build_output)) | ||
| .expect_probe_target_arch_using_cargo_rustc(&cwd, target_arch, None) | ||
| .expect_package_task_steps_with_sign_mode_off(driver_name, driver_type, target_arch); | ||
|
|
||
| assert_build_action_run_with_env_is_success( | ||
| &cwd, | ||
| profile, | ||
| None, | ||
| verify_signature, | ||
| sample_class, | ||
| test_build_action, | ||
| ); | ||
| } | ||
|
|
||
| // Given: A sample-class driver project | ||
| // When: --sign-mode=off is provided alongside --sample | ||
| // Then: It builds successfully, signing is skipped, and infverif still runs | ||
| // with the /msft sample-class flag (i.e., sample-class processing is | ||
| // independent of signing). | ||
| #[test] | ||
| pub fn given_a_sample_class_driver_project_when_sign_mode_is_off_then_signing_is_skipped_and_sample_infverif_still_runs() | ||
| { | ||
| // Input CLI args | ||
| let cwd = PathBuf::from("C:\\tmp"); | ||
| let profile = None; | ||
| let target_arch = CpuArchitecture::Amd64; | ||
| let verify_signature = false; | ||
| let sample_class = true; | ||
|
|
||
| // Driver project data | ||
| let driver_type = "KMDF"; | ||
| let driver_name = "sample-kmdf"; | ||
| let driver_version = "0.0.1"; | ||
| let wdk_metadata = get_cargo_metadata_wdk_metadata(driver_type, 1, 33); | ||
| let (workspace_member, package) = | ||
| get_cargo_metadata_package(&cwd, driver_name, driver_version, Some(&wdk_metadata)); | ||
|
|
||
| let cargo_build_output = | ||
| create_cargo_build_output_json(driver_name, driver_version, &cwd, None, profile); | ||
| let test_build_action = &TestBuildAction::new(cwd.clone(), profile, None, sample_class) | ||
| .with_sign_mode(SignMode::Off) | ||
| .set_up_standalone_driver_project((workspace_member, package)) | ||
| .expect_default_build_task_steps(driver_name, Some(cargo_build_output)) | ||
| .expect_probe_target_arch_using_cargo_rustc(&cwd, target_arch, None) | ||
| .expect_package_task_steps_with_sign_mode_off(driver_name, driver_type, target_arch) | ||
| .expect_detect_wdk_build_number(25100u32); | ||
|
|
||
| assert_build_action_run_with_env_is_success( | ||
| &cwd, | ||
| profile, | ||
| None, | ||
| verify_signature, | ||
| sample_class, | ||
| test_build_action, | ||
| ); | ||
| } | ||
|
|
||
| // Given: A driver project | ||
| // When: --sign-mode=off and --verify-signature=true are both provided | ||
| // Then: Defensive guard in PackageTask::run() takes effect — signtool verify | ||
| // is skipped (the CLI normally rejects this combo, but the package task | ||
| // itself must not invoke verification when there is nothing signed). | ||
| #[test] | ||
| pub fn given_a_driver_project_when_sign_mode_is_off_and_verify_signature_is_true_then_verification_is_skipped() | ||
| { | ||
| // Input CLI args | ||
| let cwd = PathBuf::from("C:\\tmp"); | ||
| let profile = None; | ||
| let target_arch = CpuArchitecture::Amd64; | ||
| let verify_signature = true; | ||
| let sample_class = false; | ||
|
|
||
| // Driver project data | ||
| let driver_type = "KMDF"; | ||
| let driver_name = "sample-kmdf"; | ||
| let driver_version = "0.0.1"; | ||
| let wdk_metadata = get_cargo_metadata_wdk_metadata(driver_type, 1, 33); | ||
| let (workspace_member, package) = | ||
| get_cargo_metadata_package(&cwd, driver_name, driver_version, Some(&wdk_metadata)); | ||
|
|
||
| let cargo_build_output = | ||
| create_cargo_build_output_json(driver_name, driver_version, &cwd, None, profile); | ||
| let test_build_action = &TestBuildAction::new(cwd.clone(), profile, None, sample_class) | ||
| .with_sign_mode(SignMode::Off) | ||
| .set_up_standalone_driver_project((workspace_member, package)) | ||
| .expect_default_build_task_steps(driver_name, Some(cargo_build_output)) | ||
| .expect_probe_target_arch_using_cargo_rustc(&cwd, target_arch, None) | ||
| .expect_package_task_steps_with_sign_mode_off(driver_name, driver_type, target_arch); | ||
|
|
||
| assert_build_action_run_with_env_is_success( | ||
| &cwd, | ||
| profile, | ||
| None, | ||
| verify_signature, | ||
| sample_class, | ||
| test_build_action, | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| pub fn given_a_driver_project_when_self_signed_exists_then_it_should_skip_calling_makecert() { | ||
| // Input CLI args | ||
|
|
@@ -1600,6 +1727,7 @@ fn initialize_build_action<'a>( | |
| profile, | ||
| target_arch, | ||
| verify_signature, | ||
| sign_mode: test_build_action.sign_mode, | ||
| is_sample_class: sample_class, | ||
| verbosity_level: clap_verbosity_flag::Verbosity::new(1, 0), | ||
| }, | ||
|
|
@@ -1662,6 +1790,7 @@ struct TestBuildAction { | |
| profile: Option<Profile>, | ||
| target_arch: Option<CpuArchitecture>, | ||
| sample_class: bool, | ||
| sign_mode: SignMode, | ||
|
|
||
| cargo_metadata: Option<CargoMetadata>, | ||
| // mocks | ||
|
|
@@ -1688,6 +1817,7 @@ impl TestBuildAction { | |
| profile, | ||
| target_arch, | ||
| sample_class, | ||
| sign_mode: SignMode::Test, | ||
| mock_run_command, | ||
| mock_wdk_build_provider, | ||
| mock_fs_provider, | ||
|
|
@@ -1696,6 +1826,11 @@ impl TestBuildAction { | |
| } | ||
| } | ||
|
|
||
| fn with_sign_mode(mut self, sign_mode: SignMode) -> Self { | ||
| self.sign_mode = sign_mode; | ||
| self | ||
| } | ||
|
|
||
| fn set_up_standalone_driver_project( | ||
| mut self, | ||
| package_metadata: (TestMetadataWorkspaceMemberId, TestMetadataPackage), | ||
|
|
@@ -1826,6 +1961,28 @@ impl TestBuildAction { | |
| .expect_signtool_verify_cat_file(driver_name, &cwd, None) | ||
| } | ||
|
|
||
| /// Sets up package-task expectations for `SignMode::Off`: stampinf, | ||
| /// inf2cat, and infverif are still expected, but all certificate | ||
| /// generation, signing, and signature-verification steps are skipped. | ||
| fn expect_package_task_steps_with_sign_mode_off( | ||
| self, | ||
| driver_name: &str, | ||
| driver_type: &str, | ||
| target_arch: CpuArchitecture, | ||
| ) -> Self { | ||
| let cwd = self.cwd.clone(); | ||
| self.expect_final_package_dir_exists(driver_name, &cwd, true) | ||
| .expect_inx_file_exists(driver_name, &cwd, true) | ||
| .expect_rename_driver_binary_dll_to_sys(driver_name, &cwd) | ||
| .expect_copy_driver_binary_sys_to_package_folder(driver_name, &cwd, true) | ||
| .expect_copy_pdb_file_to_package_folder(driver_name, &cwd, true) | ||
| .expect_copy_inx_file_to_package_folder(driver_name, &cwd, true, &cwd) | ||
| .expect_copy_map_file_to_package_folder(driver_name, &cwd, true) | ||
| .expect_stampinf(driver_name, &cwd, target_arch, None) | ||
| .expect_inf2cat(driver_name, &cwd, target_arch, None) | ||
| .expect_infverif(driver_name, &cwd, driver_type, None) | ||
| } | ||
|
|
||
| fn expect_default_package_task_steps_for_workspace( | ||
| self, | ||
| driver_name: &str, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When
--sign-mode=off, the package directory is not cleaned (it’s only created if missing). If a previous build ran inTestmode,WDRLocalTestCert.cermay already exist in the package folder and will be left behind because the copy step is skipped, which can defeat the goal of producing a package without the test cert artifact. Consider explicitly removing any existing cert file(s) (both in the package folder and staged under target) whenSignMode::Offis selected, or recreate/clean the package directory before copying artifacts (may require adding aremove_file/remove_dir_allmethod to theFsprovider so this is testable).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checked how MSBuild/WDK handles this in
WindowsDriver.Common.targets(10.0.26100.0): all signing targets are gated onSignMode == TestSign|ProductionSign, same as this PR. There's no mode-switch cleanup. I think we should let cleanup be handled bycargo wdk cleansimilar toMSBuild'sClean. If this UX becomes an issue, I can revisit it and implement something to ensure the final package is not stale.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we adding the cert to the package? Is that something Visual Studio does too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question!
cargo-wdkactually follows thecargo-makebased approach in rust-driver-makefile.toml:452-468, which in turn was derived from WDK's MSBuild targets. Shipping the cert for "Test-Signing" workflows makes sense because the cert needs to be installed on the target (test) machine. I need to re-verify the behavior for C++ drivers in Visual Studio.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay.
Yeah, let's verify the C++ behaviour.