From 5e80d02b5e3ccea122245be5129f745853779b58 Mon Sep 17 00:00:00 2001
From: "Neal H. Walfield" <neal@sequoia-pgp.org>
Date: Tue, 8 Oct 2024 11:38:53 +0200
Subject: [PATCH] Don't extend the expiration of subkeys that are hard revoke.

  - If a subkey is hard revoked, refuse to extend the expiration.
---
 src/common/key/expire.rs                  | 27 +++++++++++++
 tests/data/keys/hard-revoked-subkey.pgp   | 46 +++++++++++++++++++++++
 tests/integration/sq_key_subkey_expire.rs | 43 +++++++++++++++++++++
 3 files changed, 116 insertions(+)
 create mode 100644 tests/data/keys/hard-revoked-subkey.pgp

diff --git a/src/common/key/expire.rs b/src/common/key/expire.rs
index c46843ca..0fbade14 100644
--- a/src/common/key/expire.rs
+++ b/src/common/key/expire.rs
@@ -9,6 +9,7 @@ use openpgp::Result;
 use openpgp::packet::signature::SignatureBuilder;
 use openpgp::serialize::Serialize;
 use openpgp::types::RevocationStatus;
+use openpgp::types::RevocationType;
 use openpgp::types::SignatureType;
 
 use sequoia_cert_store::StoreUpdate;
@@ -79,6 +80,32 @@ pub fn expire(sq: Sq,
         // To update subkey expiration times, create new binding
         // signatures.
         for ka in subkeys {
+            if let RevocationStatus::Revoked(sigs)
+                = ka.revocation_status(sq.policy, sq.time)
+            {
+                for sig in sigs {
+                    let reason = sig.reason_for_revocation();
+                    let bad = if let Some((reason, _)) = reason {
+                        if reason.revocation_type()
+                            == RevocationType::Hard
+                        {
+                            true
+                        } else {
+                            false
+                        }
+                    } else {
+                        true
+                    };
+
+                    if bad {
+                        return Err(anyhow::anyhow!(
+                            "Can't extend the expiration of {}, \
+                             it is hard revoked",
+                            ka.fingerprint()));
+                    }
+                }
+            }
+
             // Preferably use the binding signature under our policy.
             let template = match ka.binding_signature(sq.policy, sq.time) {
                 Ok(sig) => {
diff --git a/tests/data/keys/hard-revoked-subkey.pgp b/tests/data/keys/hard-revoked-subkey.pgp
new file mode 100644
index 00000000..dc26b15c
--- /dev/null
+++ b/tests/data/keys/hard-revoked-subkey.pgp
@@ -0,0 +1,46 @@
+-----BEGIN PGP PRIVATE KEY BLOCK-----
+
+xVgEZwT5+xYJKwYBBAHaRw8BAQdADtymDG7nREpVKoDVZ3pLBMUdt1Ik/Il9/j0c
+TJ88ul0AAP0Y64rUcb6x9/YyESUlVlhkY+fZcwRwbOISaSoVgh3WvxAQwsALBB8W
+CgB9BYJnBPn7AwsJBwkQtvKvVaJ8ZnpHFAAAAAAAHgAgc2FsdEBub3RhdGlvbnMu
+c2VxdW9pYS1wZ3Aub3Jn6e2+SW9hn6Qmo8BRlSAopDT31YQevGYxfnJolGIPwRsD
+FQoIApsBAh4BFiEEDfT/jxdAVcStREHJtvKvVaJ8ZnoAAGMqAP9MrGk1mHlGtglN
+aZ1zin/4DCyTWPX1wG+2/pWifHD8cwD+OKmeMmXpJxkBTsglbWShhXiG69LKt7m5
+ysPKgxVW3Q3NNUhhcmQgcmV2b2tlZCBzdWJrZXkgPGhhcmQtcmV2b2tlZC1zdWJr
+ZXlAZXhhbXBsZS5vcmc+wsAOBBMWCgCABYJnBPn7AwsJBwkQtvKvVaJ8ZnpHFAAA
+AAAAHgAgc2FsdEBub3RhdGlvbnMuc2VxdW9pYS1wZ3Aub3JnRz+cMHmwoNsGARTQ
+5G+Ti4zyH/kGCrQff9HoJx9oPfgDFQoIApkBApsBAh4BFiEEDfT/jxdAVcStREHJ
+tvKvVaJ8ZnoAABrYAP9qHx7t1zlCOWyGzLYLRU+LPt//pJGBdxXYPHes+Dw3/QD9
+FICKWu7d7zW4KS1jRWWl43iAqZsxPCUqMcp62SilOQbHWARnBPn7FgkrBgEEAdpH
+DwEBB0BHMA5G6trQheeBrUZHrCbdK8lFJgSW5r6kCHbpPVqM3AABAN+I6BOmqBHz
+Y3fz/aNtfwcNHfFtPlRvqt3fV8ExcLrtEWTCwAQEKBYKAHYFgmcE+kUJELbyr1Wi
+fGZ6RxQAAAAAAB4AIHNhbHRAbm90YXRpb25zLnNlcXVvaWEtcGdwLm9yZ8irpriZ
+3WHycxhkaYW6f/J7w0q3Lbz6U2Jdv6bGGEyHBh0Cb3VjaBYhBA30/48XQFXErURB
+ybbyr1WifGZ6AADL7AEAyf+D3erW5br63vuUU8ybKrMHEIJmMistApsuA8ALhXIA
+/iVuZagwUT02mztzSRe7heJ2jKRP0kgFSPgAuqna3QUFwsC/BBgWCgExBYJnBPn7
+CRC28q9VonxmekcUAAAAAAAeACBzYWx0QG5vdGF0aW9ucy5zZXF1b2lhLXBncC5v
+cmfR67vm0GLFKuKAkq9oO/JcZBFkJgXEn9+sh3SJ3IumkQKbAr6gBBkWCgBvBYJn
+BPn7CRBBPTLxUmuIGUcUAAAAAAAeACBzYWx0QG5vdGF0aW9ucy5zZXF1b2lhLXBn
+cC5vcmcRCQThMxArP2lEEtDFmSV+zanpKZyHC3hQnLnNIqv1PRYhBMk1QHXkWThy
+RNh1nEE9MvFSa4gZAADoGgD7BreeLCWms0XW5wo/j7WJuJI2wlEHmh/yK1qP0LLS
+FP0BAJ1mwaNtNHMOCMVbOXavfU3L5BcE6GkC9Sq4kyhAWAMFFiEEDfT/jxdAVcSt
+REHJtvKvVaJ8ZnoAAPGcAP9wXOAlKaUvqWFpLs0Rxu6tiVLQkNIWuhEd2vNEaQcD
+oQEAo2Q4AS6JlK1ufSfYBOgyR4sPgYuCvlfKDp3HiOSQLAXHWARnBPn7FgkrBgEE
+AdpHDwEBB0DFaYcmxWAARSDn7I52uTCnnjp5UASwD3sZVS4nYOGbdgABAL3xaf5A
+4bw3mxAK87iQkvgzpfXSE4VW2k9IGgsH6u69EWPCwL8EGBYKATEFgmcE+fsJELby
+r1WifGZ6RxQAAAAAAB4AIHNhbHRAbm90YXRpb25zLnNlcXVvaWEtcGdwLm9yZ4tm
+XFY6xG+XSnDos6ZSwI8kX8Z1phCb62ETOw0wzM2vApsgvqAEGRYKAG8FgmcE+fsJ
+EPf2PJoCJOHcRxQAAAAAAB4AIHNhbHRAbm90YXRpb25zLnNlcXVvaWEtcGdwLm9y
+ZyWDEjy/t3kSp+Lq+Okd5cwHaEttOUUw/l+5Sg62q8F2FiEEiwt1DxfHMB10H0Ao
+9/Y8mgIk4dwAAHi4AQCKZALWJaA0bGP7xpo+MIMGMilI5fGA3C8uVJAz3XAcHQD+
+LtAi3EcSiNxUUDAw6NXvkin0Qp+WGv4MBaSAXQi4jAEWIQQN9P+PF0BVxK1EQcm2
+8q9VonxmegAAHHYA/1kIB4BjVYM2PC63WhtY8hlRfryiU/ylZYVZz/7I9a75AP9w
+gyrtQ0vq+UWuTxtnQVi5PMTWnZzldQvcZGhyFVbFA8ddBGcE+fsSCisGAQQBl1UB
+BQEBB0DU1sVsaYBRTIwJqZXv+CPGXJG4qEHvLYBmQTy04RctRAMBCAcAAP9WA2lC
+si6WcmZ5gIdn6HLPCZIbYYLRFYovA4OOjv8ZYA6zwsAABBgWCgByBYJnBPn7CRC2
+8q9VonxmekcUAAAAAAAeACBzYWx0QG5vdGF0aW9ucy5zZXF1b2lhLXBncC5vcmfy
+elIlTrKZ5YtKnWT8VsXfJIaTUi+N0tsT0EfFTYGlIAKbDBYhBA30/48XQFXErURB
+ybbyr1WifGZ6AAB+GgD9GBVTpXsdSe/FccIUUD1gjQEyIoafXNMKsfc7Tqnb7qIA
+/RIZcuXhwzQQuedi53sfw2cvs6rMiEhUaxVhiI+FQ94D
+=Fcvc
+-----END PGP PRIVATE KEY BLOCK-----
diff --git a/tests/integration/sq_key_subkey_expire.rs b/tests/integration/sq_key_subkey_expire.rs
index 87496126..df3b4d90 100644
--- a/tests/integration/sq_key_subkey_expire.rs
+++ b/tests/integration/sq_key_subkey_expire.rs
@@ -280,3 +280,46 @@ fn soft_revoked_subkey() {
     }
     assert!(good);
 }
+
+#[test]
+fn hard_revoked_subkey() {
+    // Make sure we can't extend the expiration time of a hard revoked
+    // subkey.
+
+    let sq = Sq::new();
+
+    let cert_path = sq.test_data()
+        .join("keys")
+        .join("hard-revoked-subkey.pgp");
+
+    let cert = Cert::from_file(&cert_path).expect("can read");
+    let vc = cert.with_policy(STANDARD_POLICY, sq.now())
+        .expect("valid cert");
+
+    // Make sure the revoked key is there and is really revoked.
+    let mut revoked = None;
+    for k in vc.keys().subkeys() {
+        if let RevocationStatus::Revoked(_) = k.revocation_status() {
+            assert!(revoked.is_none(),
+                    "Only expected a single revoked subkey");
+            revoked = Some(k.fingerprint());
+        }
+    }
+    let revoked = if let Some(revoked) = revoked {
+        revoked
+    } else {
+        panic!("Expected a revoked subkey, but didn't fine one");
+    };
+
+    // Set it to expire in a day.
+    let updated_path = sq.scratch_file("updated");
+    let result = sq.key_subkey_expire(cert_path,
+                                       &[ revoked.clone().into() ],
+                                       "1d",
+                                       None,
+                                       updated_path.as_path(),
+                                       false);
+    if result.is_ok() {
+        panic!("Updated expiration of hard revoked subkey, but shouldn't have.");
+    }
+}