Merge pull request #3833 from reubenmiller/test-p11-1.6-incompat-cert-renew

Bravo555 · web-flow · commit 43a8d11e06c5 · 2025-11-05T10:19:39.000Z
fix: Fix `tedge cert renew` incompatibility when using 1.6.1 `tedge-p11-server` and more recent `tedge`
diff --git a/crates/common/certificate/src/lib.rs b/crates/common/certificate/src/lib.rs
@@ -155,7 +155,7 @@ pub enum KeyKind {
 }
 
 impl KeyKind {
-    pub fn from_cryptoki_and_existing_cert(
+    fn from_cryptoki_and_existing_cert(
         cryptoki_config: CryptokiConfig,
         current_cert: &Utf8Path,
     ) -> Result<Self, CertificateError> {
@@ -193,9 +193,29 @@ impl KeyKind {
         }))
     }
 
-    pub fn from_cryptoki(cryptoki_config: CryptokiConfig) -> Result<Self, CertificateError> {
+    pub fn from_cryptoki(
+        cryptoki_config: CryptokiConfig,
+        current_cert: Option<&Utf8Path>,
+    ) -> Result<Self, CertificateError> {
         let cryptoki = tedge_p11_server::tedge_p11_service(cryptoki_config.clone())?;
-        let pubkey_pem = cryptoki.get_public_key_pem(None)?;
+
+        let pubkey_pem = cryptoki.get_public_key_pem(None);
+        let pubkey_pem = match pubkey_pem {
+            Ok(p) => p,
+            Err(err) => {
+                let e = format!("{err:#}");
+                if e.contains("Failed to parse the received frame") {
+                    // server doesn't understand the request, too old, fallback to the older method of just resigning existing certificate
+                    let Some(current_cert) = current_cert else {
+                        return Err(CertificateError::Other(
+                            anyhow::anyhow!("tedge-p11-server can only renew existing certificates but there's no existing certificate; upgrade tedge-p11-server or generate a self-signed certificate first")));
+                    };
+                    return Self::from_cryptoki_and_existing_cert(cryptoki_config, current_cert);
+                }
+                return Err(err.into());
+            }
+        };
+
         let public_key = pem::parse(&pubkey_pem).unwrap();
         let public_key_raw = public_key.into_contents();
 
diff --git a/crates/core/tedge/src/cli/certificate/c8y/download.rs b/crates/core/tedge/src/cli/certificate/c8y/download.rs
@@ -85,6 +85,7 @@ impl DownloadCertCmd {
             create_device_csr(
                 common_name.clone(),
                 self.key.clone(),
+                None,
                 self.csr_path.clone(),
                 self.csr_template.clone(),
             )
diff --git a/crates/core/tedge/src/cli/certificate/c8y/mod.rs b/crates/core/tedge/src/cli/certificate/c8y/mod.rs
@@ -18,13 +18,15 @@ pub use upload::UploadCertCmd;
 async fn create_device_csr(
     common_name: String,
     key: super::create_csr::Key,
+    current_cert: Option<Utf8PathBuf>,
     csr_path: Utf8PathBuf,
     csr_template: CsrTemplate,
 ) -> Result<(), CertError> {
     let create_cmd = CreateCsrCmd {
         id: common_name,
         csr_path: csr_path.clone(),
         key,
+        current_cert,
         user: "tedge".to_string(),
         group: "tedge".to_string(),
         csr_template,
diff --git a/crates/core/tedge/src/cli/certificate/c8y/renew.rs b/crates/core/tedge/src/cli/certificate/c8y/renew.rs
@@ -84,6 +84,7 @@ impl RenewCertCmd {
             create_device_csr(
                 common_name,
                 self.key.clone(),
+                Some(self.cert_path.clone()),
                 self.csr_path.clone(),
                 self.csr_template.clone(),
             )
diff --git a/crates/core/tedge/src/cli/certificate/cli.rs b/crates/core/tedge/src/cli/certificate/cli.rs
@@ -197,6 +197,11 @@ impl BuildCommand for TEdgeCertCli {
                         config.device_key_path(cloud.as_ref())?.to_owned(),
                     ));
                 debug!(?key);
+                let current_cert = config
+                    .device_cert_path(cloud.as_ref())
+                    .map(|c| c.to_owned())
+                    .ok();
+                debug!(?current_cert);
 
                 let cmd = CreateCsrCmd {
                     id: get_device_id(id, config, &cloud)?,
@@ -207,6 +212,7 @@ impl BuildCommand for TEdgeCertCli {
                     } else {
                         config.device_csr_path(cloud.as_ref())?.to_owned()
                     },
+                    current_cert,
                     user: user.to_owned(),
                     group: group.to_owned(),
                     csr_template,
diff --git a/crates/core/tedge/src/cli/certificate/create_csr.rs b/crates/core/tedge/src/cli/certificate/create_csr.rs
@@ -24,6 +24,9 @@ pub struct CreateCsrCmd {
     /// The path where the device CSR will be stored
     pub csr_path: Utf8PathBuf,
 
+    /// Path to current certificate, if it exists
+    pub current_cert: Option<Utf8PathBuf>,
+
     /// The owner of the private key
     pub user: String,
     pub group: String,
@@ -63,7 +66,10 @@ impl CreateCsrCmd {
                 .await
                 .map_err(|e| CertError::IoError(e).key_context(key_path.clone()))?,
 
-            Key::Cryptoki(config) => KeyKind::from_cryptoki(config.clone())?,
+            Key::Cryptoki(config) => KeyKind::from_cryptoki(
+                config.clone(),
+                self.current_cert.as_ref().map(|p| p.as_path()),
+            )?,
         };
         debug!(?previous_key);
 
@@ -111,6 +117,7 @@ mod tests {
             user: "mosquitto".to_string(),
             group: "mosquitto".to_string(),
             csr_template: CsrTemplate::default(),
+            current_cert: None,
         };
 
         assert_matches!(cmd.create_certificate_signing_request().await, Ok(()));
@@ -154,6 +161,7 @@ mod tests {
             user: "mosquitto".to_string(),
             group: "mosquitto".to_string(),
             csr_template: CsrTemplate::default(),
+            current_cert: None,
         };
 
         // create csr using existing private key and device_id from public cert
diff --git a/tests/RobotFramework/tests/pkcs11/compatibility_16x.robot b/tests/RobotFramework/tests/pkcs11/compatibility_16x.robot
@@ -0,0 +1,60 @@
+*** Settings ***
+Documentation       This test suite runs the tests with tedge-p11-server pinned to a fixed version to ensure that new
+...                 versions of thin-edge remain backwards compatible with tedge-p11-server's binary communication protocol. The
+...                 scope of this test is limited to tedge-p11-server's initial feature set and will generally not be expanded.
+
+Resource            pkcs11_common.resource
+
+Suite Setup         Custom Setup
+Suite Teardown      Get Suite Logs
+
+Test Tags           adapter:docker    theme:cryptoki    compatibility
+
+
+*** Variables ***
+${TEDGE_P11_SERVER_VERSION}     1.6.1
+
+
+*** Test Cases ***
+# the test cases are basically copy-pasted from private_key_storage.robot, as the purpose of this suite is to run the
+# exact same tests with a slightly different setup. It would be easiest if we could import the test cases themselves
+# from another test suite, but this isn't possible. So we extract reusable keywords into a resource file, but test cases
+# remain duplicated.
+Use Private Key in SoftHSM2 using tedge-p11-server
+    Tedge Reconnect Should Succeed
+
+Renew certificate
+    Execute Command    tedge cert renew c8y
+    Tedge Reconnect Should Succeed
+
+
+*** Keywords ***
+Custom Setup
+    ${DEVICE_SN}=    Setup    register=${False}
+    Set Suite Variable    ${DEVICE_SN}
+
+    # this doesn't install anything but adds cloudsmith repo to apt
+    Execute Command    curl -1sLf 'https://dl.cloudsmith.io/public/thinedge/tedge-main/setup.deb.sh' | sudo -E bash
+    Execute Command    cmd=apt-get install -y --allow-downgrades tedge-p11-server=${TEDGE_P11_SERVER_VERSION}
+    ${stdout}=    Execute Command    tedge-p11-server -V    strip=True
+    Should Be Equal    ${stdout}    tedge-p11-server ${TEDGE_P11_SERVER_VERSION}
+
+    # Allow the tedge user to access softhsm
+    Execute Command    sudo usermod -a -G softhsm tedge
+    Transfer To Device    ${CURDIR}/data/init_softhsm.sh    /usr/bin/
+
+    # initialize the soft hsm and create a certificate signing request
+    Execute Command    tedge config set device.cryptoki.pin 123456
+    Execute Command    tedge config set device.cryptoki.module_path /usr/lib/softhsm/libsofthsm2.so
+    Execute Command    sudo -u tedge /usr/bin/init_softhsm.sh --device-id "${DEVICE_SN}" --pin 123456
+
+    # configure tedge
+    ${domain}=    Cumulocity.Get Domain
+    Execute Command    tedge config set c8y.url "${domain}"
+    Execute Command    tedge config set mqtt.bridge.built_in true
+    Execute Command    tedge config set device.cryptoki.mode socket
+
+    ${csr_path}=    Execute Command    cmd=tedge config get device.csr_path    strip=${True}
+    Register Device With Cumulocity CA    ${DEVICE_SN}    csr_path=${csr_path}
+
+    Unset tedge-p11-server Uri

Original file line number	Diff line number	Diff line change
`@@ -85,6 +85,7 @@ impl DownloadCertCmd {`
`85`	`85`	`create_device_csr(`
`86`	`86`	`common_name.clone(),`
`87`	`87`	`self.key.clone(),`
	`88`	`+ None,`
`88`	`89`	`self.csr_path.clone(),`
`89`	`90`	`self.csr_template.clone(),`
`90`	`91`	`)`
Original file line number	Diff line number	Diff line change
`@@ -84,6 +84,7 @@ impl RenewCertCmd {`
`84`	`84`	`create_device_csr(`
`85`	`85`	`common_name,`
`86`	`86`	`self.key.clone(),`
	`87`	`+ Some(self.cert_path.clone()),`
`87`	`88`	`self.csr_path.clone(),`
`88`	`89`	`self.csr_template.clone(),`
`89`	`90`	`)`