apple-codesign: fold session initialization arguments into remote sig…

…ning ones The multiple levels of nesting combined with Option<T> were conspiring to prevent CLI arguments from getting processed. I believe this is clap-rs/clap#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.
indygreg · Nov 16, 2023 · d0bfbe2 · d0bfbe2
1 parent 1453147
commit d0bfbe2
Show file tree

Hide file tree

Showing 11 changed files with 56 additions and 112 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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<String>,
+
     /// 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<String>,
 }
 
-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<SigningCertificates, AppleCodesignError> {
-        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<Box<dyn SessionInitiatePeer>, 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<Option<Box<dyn SessionInitiatePeer>>, 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
@@ -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
@@ -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
@@ -57,13 +57,8 @@ Options:
       --p12-password-file <PATH>
           Path to file containing password for opening --p12-file file
 
-      --remote-signer
-          Send signing requests to a remote signer
-
       --remote-signing-url <URL>
           URL of a remote code signing server
-
-          [default: wss://ws.codesign.gregoryszorc.com/]
 
       --remote-public-key <BASE64 ENCODED 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
@@ -56,13 +56,8 @@ Options:
       --p12-password-file <PATH>
           Path to file containing password for opening --p12-file file
 
-      --remote-signer
-          Send signing requests to a remote signer
-
       --remote-signing-url <URL>
           URL of a remote code signing server
-
-          [default: wss://ws.codesign.gregoryszorc.com/]
 
       --remote-public-key <BASE64 ENCODED 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
@@ -63,13 +63,8 @@ Options:
       --p12-password-file <PATH>
           Path to file containing password for opening --p12-file file
 
-      --remote-signer
-          Send signing requests to a remote signer
-
       --remote-signing-url <URL>
           URL of a remote code signing server
-
-          [default: wss://ws.codesign.gregoryszorc.com/]
 
       --remote-public-key <BASE64 ENCODED 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
@@ -361,13 +361,8 @@ Options:
       --p12-password-file <PATH>
           Path to file containing password for opening --p12-file file
 
-      --remote-signer
-          Send signing requests to a remote signer
-
       --remote-signing-url <URL>
           URL of a remote code signing server
-
-          [default: wss://ws.codesign.gregoryszorc.com/]
 
       --remote-public-key <BASE64 ENCODED 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
@@ -59,13 +59,8 @@ Options:
       --p12-password-file <PATH>
           Path to file containing password for opening --p12-file file
 
-      --remote-signer
-          Send signing requests to a remote signer
-
       --remote-signing-url <URL>
           URL of a remote code signing server
-
-          [default: wss://ws.codesign.gregoryszorc.com/]
 
       --remote-public-key <BASE64 ENCODED PUBLIC KEY>
           Base64 encoded public key data describing the signer