From d0bfbe22229602277c3c5d9e96756d2bb1d092e0 Mon Sep 17 00:00:00 2001 From: Gregory Szorc Date: Thu, 16 Nov 2023 10:31:25 +0800 Subject: [PATCH] apple-codesign: fold session initialization arguments into remote signing ones The multiple levels of nesting combined with Option were conspiring to prevent CLI arguments from getting processed. I believe this is https://github.com/clap-rs/clap/issues/4697 but am not 100% sure. We mitigate the problem by moving the session initialization fields into the main key struct so we have a single level of argument nesting. As part of this we remote `--remote-signer` and instead infer remote signing from presence of a session initialization argument. We also make the server URL optional to prevent the struct from having any populated fields by default. --- Justfile | 2 - apple-codesign/CHANGELOG.md | 9 ++ .../apple_codesign_rcodesign_config_files.rst | 6 +- apple-codesign/src/cli/certificate_source.rs | 88 +++++++------------ apple-codesign/src/cli/config.rs | 32 +++---- apple-codesign/src/cli/mod.rs | 6 +- .../tests/cmd/analyze-certificate.trycmd | 5 -- apple-codesign/tests/cmd/generate-csr.trycmd | 5 -- apple-codesign/tests/cmd/remote-sign.trycmd | 5 -- apple-codesign/tests/cmd/sign.trycmd | 5 -- .../tests/cmd/smartcard-import.trycmd | 5 -- 11 files changed, 56 insertions(+), 112 deletions(-) diff --git a/Justfile b/Justfile index 571cb15cb..7dd84516e 100644 --- a/Justfile +++ b/Justfile @@ -65,14 +65,12 @@ assemble-exe-artifacts exe commit dest: _codesign-exe in_path: rcodesign sign \ - --remote-signer \ --remote-public-key-pem-file ci/developer-id-application.pem \ --code-signature-flags runtime \ {{in_path}} _codesign in_path out_path: rcodesign sign \ - --remote-signer \ --remote-public-key-pem-file ci/developer-id-application.pem \ {{in_path}} {{out_path}} diff --git a/apple-codesign/CHANGELOG.md b/apple-codesign/CHANGELOG.md index f6eeb1b03..34b099d66 100644 --- a/apple-codesign/CHANGELOG.md +++ b/apple-codesign/CHANGELOG.md @@ -6,10 +6,19 @@ Released on ReleaseDate. +* (Breaking change) The `sign --remote-signer` argument has been removed. It + is now implicitly assumed via presence of a remote session initialization + argument. +* Fixed a regression in 0.25.0 where remote signing didn't work due argument + parsing errors. + ## 0.25.0 Released on 2023-11-15. +(Binary assets for this release were never formally published due to a +regression in remote signing CLI argument handling.) + * (Breaking change) The `--extra-digest` argument has been removed. `--digest` can now be specified multiple times. `--digest` is now a scoped value. diff --git a/apple-codesign/docs/apple_codesign_rcodesign_config_files.rst b/apple-codesign/docs/apple_codesign_rcodesign_config_files.rst index 967a21c65..5d45066b1 100644 --- a/apple-codesign/docs/apple_codesign_rcodesign_config_files.rst +++ b/apple-codesign/docs/apple_codesign_rcodesign_config_files.rst @@ -324,15 +324,15 @@ This key is a table/dict/map with the following keys: Leave blank to use the default. -``session_init.public_key`` +``public_key`` Base64 encoded public key data used to encrypt a message to the remote signer. -``session_init.public_key_pem_path`` +``public_key_pem_path`` File containing PEM encoded public key data used to encrypt a message to the remote signer. -``session_init.shared_secret`` +``shared_secret`` A shared secret value (i.e. a password/passphrase) used to encrypt a message to the remote signer. diff --git a/apple-codesign/src/cli/certificate_source.rs b/apple-codesign/src/cli/certificate_source.rs index baca54b3c..6e65db927 100644 --- a/apple-codesign/src/cli/certificate_source.rs +++ b/apple-codesign/src/cli/certificate_source.rs @@ -361,7 +361,11 @@ impl KeySource for PemSigningKey { #[derive(Args, Clone, Debug, Default, Deserialize, Eq, PartialEq, Serialize)] #[serde(deny_unknown_fields)] -pub struct RemoteSigningSessionInitialization { +pub struct RemoteSigningKey { + /// URL of a remote code signing server + #[arg(long = "remote-signing-url", value_name = "URL")] + pub url: Option, + /// Base64 encoded public key data describing the signer #[arg( long = "remote-public-key", @@ -396,44 +400,11 @@ pub struct RemoteSigningSessionInitialization { pub shared_secret_env: Option, } -fn default_remote_signing_url() -> String { - crate::remote_signing::DEFAULT_SERVER_URL.to_string() -} - -#[derive(Args, Clone, Debug, Deserialize, Eq, PartialEq, Serialize)] -#[serde(deny_unknown_fields)] -pub struct RemoteSigningKey { - /// Send signing requests to a remote signer - #[arg(long = "remote-signer")] - #[serde(default, skip)] - pub enabled: bool, - - /// URL of a remote code signing server - #[arg(long = "remote-signing-url", default_value = crate::remote_signing::DEFAULT_SERVER_URL, value_name = "URL")] - #[serde(default = "default_remote_signing_url")] - pub url: String, - - #[command(flatten)] - pub session_init: RemoteSigningSessionInitialization, -} - -impl Default for RemoteSigningKey { - fn default() -> Self { - Self { - enabled: false, - url: crate::remote_signing::DEFAULT_SERVER_URL.to_string(), - session_init: RemoteSigningSessionInitialization::default(), - } - } -} - impl KeySource for RemoteSigningKey { fn resolve_certificates(&self) -> Result { - if self.enabled { - let initiator = self.remote_signing_initiator()?; - + if let Some(initiator) = self.remote_signing_initiator()? { let client = UnjoinedSigningClient::new_initiator( - self.url.clone(), + self.url(), initiator, Some(super::print_session_join), )?; @@ -456,17 +427,26 @@ impl KeySource for RemoteSigningKey { } impl RemoteSigningKey { - fn remote_signing_initiator(&self) -> Result, RemoteSignError> { - let server_url = self.url.clone(); + /// Obtain the URL of the relay server. + pub fn url(&self) -> String { + self.url + .clone() + .unwrap_or_else(|| crate::remote_signing::DEFAULT_SERVER_URL.to_string()) + } - if let Some(public_key_data) = &self.session_init.public_key { + fn remote_signing_initiator( + &self, + ) -> Result>, RemoteSignError> { + let server_url = self.url(); + + if let Some(public_key_data) = &self.public_key { let public_key_data = STANDARD_ENGINE.decode(public_key_data)?; - Ok(Box::new(PublicKeyInitiator::new( + Ok(Some(Box::new(PublicKeyInitiator::new( public_key_data, Some(server_url), - )?)) - } else if let Some(path) = &self.session_init.public_key_pem_path { + )?))) + } else if let Some(path) = &self.public_key_pem_path { let pem_data = std::fs::read(path)?; let doc = pem::parse(pem_data)?; @@ -485,32 +465,26 @@ impl RemoteSigningKey { } }; - Ok(Box::new(PublicKeyInitiator::new( + Ok(Some(Box::new(PublicKeyInitiator::new( spki_der, Some(server_url), - )?)) - } else if let Some(env) = &self.session_init.shared_secret_env { + )?))) + } else if let Some(env) = &self.shared_secret_env { let secret = std::env::var(env).map_err(|_| { RemoteSignError::ClientState( "failed reading from shared secret environment variable", ) })?; - Ok(Box::new(SharedSecretInitiator::new( + Ok(Some(Box::new(SharedSecretInitiator::new( secret.as_bytes().to_vec(), - )?)) - } else if let Some(value) = &self.session_init.shared_secret { - Ok(Box::new(SharedSecretInitiator::new( + )?))) + } else if let Some(value) = &self.shared_secret { + Ok(Some(Box::new(SharedSecretInitiator::new( value.as_bytes().to_vec(), - )?)) + )?))) } else { - error!("no arguments provided to establish session with remote signer"); - error!( - "specify --remote-public-key, --remote-shared-secret-env, or --remote-shared-secret" - ); - Err(RemoteSignError::ClientState( - "unable to initiate remote signing", - )) + Ok(None) } } } diff --git a/apple-codesign/src/cli/config.rs b/apple-codesign/src/cli/config.rs index cfb05d391..ad32842fa 100644 --- a/apple-codesign/src/cli/config.rs +++ b/apple-codesign/src/cli/config.rs @@ -162,7 +162,7 @@ mod test { use { crate::cli::certificate_source::{ MacosKeychainSigningKey, P12SigningKey, PemSigningKey, RemoteSigningKey, - RemoteSigningSessionInitialization, SmartcardSigningKey, + SmartcardSigningKey, }, std::path::PathBuf, }; @@ -320,7 +320,7 @@ mod test { .toml_string( r#" [default.sign] - signer.remote.session_init.public_key = "DEADBEEF" + signer.remote.public_key = "DEADBEEF" "# ) .config() @@ -329,12 +329,8 @@ mod test { .signer, CertificateSource { remote_signing_key: Some(RemoteSigningKey { - enabled: false, - url: crate::remote_signing::DEFAULT_SERVER_URL.to_string(), - session_init: RemoteSigningSessionInitialization { - public_key: Some("DEADBEEF".into()), - ..Default::default() - }, + public_key: Some("DEADBEEF".into()), + ..Default::default() }), ..Default::default() } @@ -345,7 +341,7 @@ mod test { .toml_string( r#" [default.sign] - signer.remote.session_init.public_key_pem_path = "path/to/cert.pem" + signer.remote.public_key_pem_path = "path/to/cert.pem" "# ) .config() @@ -354,12 +350,8 @@ mod test { .signer, CertificateSource { remote_signing_key: Some(RemoteSigningKey { - enabled: false, - url: crate::remote_signing::DEFAULT_SERVER_URL.to_string(), - session_init: RemoteSigningSessionInitialization { - public_key_pem_path: Some("path/to/cert.pem".into()), - ..Default::default() - }, + public_key_pem_path: Some("path/to/cert.pem".into()), + ..Default::default() }), ..Default::default() } @@ -370,7 +362,7 @@ mod test { .toml_string( r#" [default.sign] - signer.remote.session_init.shared_secret = "SECRET" + signer.remote.shared_secret = "SECRET" "# ) .config() @@ -379,12 +371,8 @@ mod test { .signer, CertificateSource { remote_signing_key: Some(RemoteSigningKey { - enabled: false, - url: crate::remote_signing::DEFAULT_SERVER_URL.to_string(), - session_init: RemoteSigningSessionInitialization { - shared_secret: Some("SECRET".into()), - ..Default::default() - }, + shared_secret: Some("SECRET".into()), + ..Default::default() }), ..Default::default() } diff --git a/apple-codesign/src/cli/mod.rs b/apple-codesign/src/cli/mod.rs index 3281e10f2..e1c93e3a5 100644 --- a/apple-codesign/src/cli/mod.rs +++ b/apple-codesign/src/cli/mod.rs @@ -972,16 +972,16 @@ impl CliCommand for RemoteSign { let mut joiner = create_session_joiner(session_join_string)?; let url = if let Some(key) = &c.signer.remote_signing_key { - if let Some(env) = &key.session_init.shared_secret_env { + if let Some(env) = &key.shared_secret_env { let secret = std::env::var(env).map_err(|_| AppleCodesignError::CliBadArgument)?; joiner .register_state(SessionJoinState::SharedSecret(secret.as_bytes().to_vec()))?; - } else if let Some(secret) = &key.session_init.shared_secret { + } else if let Some(secret) = &key.shared_secret { joiner .register_state(SessionJoinState::SharedSecret(secret.as_bytes().to_vec()))?; } - key.url.clone() + key.url() } else { crate::remote_signing::DEFAULT_SERVER_URL.to_string() }; diff --git a/apple-codesign/tests/cmd/analyze-certificate.trycmd b/apple-codesign/tests/cmd/analyze-certificate.trycmd index 0356b506e..804ef9a50 100644 --- a/apple-codesign/tests/cmd/analyze-certificate.trycmd +++ b/apple-codesign/tests/cmd/analyze-certificate.trycmd @@ -57,13 +57,8 @@ Options: --p12-password-file Path to file containing password for opening --p12-file file - --remote-signer - Send signing requests to a remote signer - --remote-signing-url URL of a remote code signing server - - [default: wss://ws.codesign.gregoryszorc.com/] --remote-public-key Base64 encoded public key data describing the signer diff --git a/apple-codesign/tests/cmd/generate-csr.trycmd b/apple-codesign/tests/cmd/generate-csr.trycmd index e84753da6..93c430f8a 100644 --- a/apple-codesign/tests/cmd/generate-csr.trycmd +++ b/apple-codesign/tests/cmd/generate-csr.trycmd @@ -56,13 +56,8 @@ Options: --p12-password-file Path to file containing password for opening --p12-file file - --remote-signer - Send signing requests to a remote signer - --remote-signing-url URL of a remote code signing server - - [default: wss://ws.codesign.gregoryszorc.com/] --remote-public-key Base64 encoded public key data describing the signer diff --git a/apple-codesign/tests/cmd/remote-sign.trycmd b/apple-codesign/tests/cmd/remote-sign.trycmd index 7e63ef0e1..b1bd5fbe9 100644 --- a/apple-codesign/tests/cmd/remote-sign.trycmd +++ b/apple-codesign/tests/cmd/remote-sign.trycmd @@ -63,13 +63,8 @@ Options: --p12-password-file Path to file containing password for opening --p12-file file - --remote-signer - Send signing requests to a remote signer - --remote-signing-url URL of a remote code signing server - - [default: wss://ws.codesign.gregoryszorc.com/] --remote-public-key Base64 encoded public key data describing the signer diff --git a/apple-codesign/tests/cmd/sign.trycmd b/apple-codesign/tests/cmd/sign.trycmd index 398a583b2..1a2bd50ca 100644 --- a/apple-codesign/tests/cmd/sign.trycmd +++ b/apple-codesign/tests/cmd/sign.trycmd @@ -361,13 +361,8 @@ Options: --p12-password-file Path to file containing password for opening --p12-file file - --remote-signer - Send signing requests to a remote signer - --remote-signing-url URL of a remote code signing server - - [default: wss://ws.codesign.gregoryszorc.com/] --remote-public-key Base64 encoded public key data describing the signer diff --git a/apple-codesign/tests/cmd/smartcard-import.trycmd b/apple-codesign/tests/cmd/smartcard-import.trycmd index 825c27f5a..2f4f42e3f 100644 --- a/apple-codesign/tests/cmd/smartcard-import.trycmd +++ b/apple-codesign/tests/cmd/smartcard-import.trycmd @@ -59,13 +59,8 @@ Options: --p12-password-file Path to file containing password for opening --p12-file file - --remote-signer - Send signing requests to a remote signer - --remote-signing-url URL of a remote code signing server - - [default: wss://ws.codesign.gregoryszorc.com/] --remote-public-key Base64 encoded public key data describing the signer