openpgp: Change CertBuilder to use a relative expiration time.

  - `CertBuilder::set_expiration_time` takes an absolute time.

    - Most callers use a relative time.

    - Internally, we need a relative time (that's what the Key
      Expiration Time packet takes).

    - Converting the absolute time to a relative time is error prone:
      should it be relative to the creation time when called or when
      `CertBuilder` is finalized?

  - KISS: Change it to just take a relative time.

  - To better reflect the new semantics, also change the name to
    `CertBuilder::set_validity_period`.

5 files changed, 46 insertions, 45 deletions

diff --git a/openpgp/src/cert/amalgamation/key.rs b/openpgp/src/cert/amalgamation/key.rs
index 4c2fc1ca..78f34f5d 100644
--- a/openpgp/src/cert/amalgamation/key.rs
+++ b/openpgp/src/cert/amalgamation/key.rs
@@ -1793,12 +1793,11 @@ impl<'a, P, R, R2> ValidKeyAmalgamation<'a, P, R, R2>
     /// let now: time::SystemTime = now.try_into()?;
     ///
     /// let a_week = time::Duration::from_secs(7 * 24 * 60 * 60);
-    /// let a_week_later = now + a_week;
     ///
     /// let (cert, _) =
     ///     CertBuilder::general_purpose(None, Some("alice@example.org"))
     ///     .set_creation_time(now)
-    ///     .set_expiration_time(a_week_later)
+    ///     .set_validity_period(a_week)
     ///     .generate()?;
     ///
     /// assert_eq!(cert.primary_key().with_policy(p, None)?.key_validity_period(),
@@ -1849,7 +1848,7 @@ impl<'a, P, R, R2> ValidKeyAmalgamation<'a, P, R, R2>
     /// let (cert, _) =
     ///     CertBuilder::general_purpose(None, Some("alice@example.org"))
     ///     .set_creation_time(now)
-    ///     .set_expiration_time(a_week_later)
+    ///     .set_validity_period(a_week)
     ///     .generate()?;
     ///
     /// assert_eq!(cert.primary_key().with_policy(p, None)?.key_expiration_time(),
diff --git a/openpgp/src/cert/builder.rs b/openpgp/src/cert/builder.rs
index ebe42d69..0750143a 100644
--- a/openpgp/src/cert/builder.rs
+++ b/openpgp/src/cert/builder.rs
@@ -125,7 +125,7 @@ impl CipherSuite {
 #[derive(Clone, Debug)]
 pub struct KeyBlueprint {
     flags: KeyFlags,
-    expiration: Option<time::SystemTime>,
+    validity: Option<time::Duration>,
     // If not None, uses the specified ciphersuite.  Otherwise, uses
     // CertBuilder::ciphersuite.
     ciphersuite: Option<CipherSuite>,
@@ -212,7 +212,7 @@ impl CertBuilder {
             ciphersuite: CipherSuite::default(),
             primary: KeyBlueprint{
                 flags: KeyFlags::default().set_certification(),
-                expiration: None,
+                validity: None,
                 ciphersuite: None,
             },
             subkeys: vec![],
@@ -257,9 +257,7 @@ impl CertBuilder {
                 flags: KeyFlags::default()
                     .set_certification()
                     .set_signing(),
-                expiration: Some(
-                    time::SystemTime::now()
-                        + time::Duration::new(3 * 52 * 7 * 24 * 60 * 60, 0)),
+                validity: Some(time::Duration::new(3 * 52 * 7 * 24 * 60 * 60, 0)),
                 ciphersuite: None,
             },
             subkeys: vec![
@@ -267,7 +265,7 @@ impl CertBuilder {
                     flags: KeyFlags::default()
                         .set_transport_encryption()
                         .set_storage_encryption(),
-                    expiration: None,
+                    validity: None,
                     ciphersuite: None,
                 }
             ],
@@ -741,11 +739,11 @@ impl CertBuilder {
     /// // expire in a year.
     /// let (cert,_) = CertBuilder::new()
     ///     .set_creation_time(now)
-    ///     .set_expiration_time(now + 2 * y)
+    ///     .set_validity_period(2 * y)
     ///     .add_subkey(KeyFlags::empty()
     ///                     .set_storage_encryption()
     ///                     .set_transport_encryption(),
-    ///                 now + y,
+    ///                 y,
     ///                 None)
     ///     .generate()?;
     ///
@@ -760,14 +758,14 @@ impl CertBuilder {
     ///                     .set_transport_encryption()));
     /// # Ok(()) }
     /// ```
-    pub fn add_subkey<T, C>(mut self, flags: KeyFlags, expiration: T, cs: C)
+    pub fn add_subkey<T, C>(mut self, flags: KeyFlags, validity: T, cs: C)
         -> Self
-        where T: Into<Option<time::SystemTime>>,
+        where T: Into<Option<time::Duration>>,
               C: Into<Option<CipherSuite>>,
     {
         self.subkeys.push(KeyBlueprint {
             flags,
-            expiration: expiration.into(),
+            validity: validity.into(),
             ciphersuite: cs.into(),
         });
         self
@@ -838,9 +836,13 @@ impl CertBuilder {
         self
     }
 
-    /// Sets the certificate's expiration time.
+    /// Sets the certificate's validity period.
     ///
-    /// A value of None means never.
+    /// The determines how long the certificate is valid.  That is,
+    /// after the validity period, the certificate is considered to be
+    /// expired.
+    ///
+    /// A value of None means that the certificate never expires.
     //
     /// # Examples
     ///
@@ -860,7 +862,7 @@ impl CertBuilder {
     /// // Make the certificate expire in 10 minutes.
     /// let (cert,_) = CertBuilder::new()
     ///     .set_creation_time(now)
-    ///     .set_expiration_time(now + 600 * s)
+    ///     .set_validity_period(600 * s)
     ///     .generate()?;
     ///
     /// assert!(cert.with_policy(p, now)?.primary_key().alive().is_ok());
@@ -868,10 +870,10 @@ impl CertBuilder {
     /// assert!(cert.with_policy(p, now + 600 * s)?.primary_key().alive().is_err());
     /// # Ok(()) }
     /// ```
-    pub fn set_expiration_time<T>(mut self, expiration: T) -> Self
-        where T: Into<Option<time::SystemTime>>
+    pub fn set_validity_period<T>(mut self, validity: T) -> Self
+        where T: Into<Option<time::Duration>>
     {
-        self.primary.expiration = expiration.into();
+        self.primary.validity = validity.into();
         self
     }
 
@@ -1018,9 +1020,7 @@ impl CertBuilder {
                 .set_hash_algo(HashAlgorithm::SHA512)
                 .set_features(&Features::sequoia())?
                 .set_key_flags(flags)?
-                .set_key_expiration_time(
-                    &subkey,
-                    blueprint.expiration.or(self.primary.expiration))?;
+                .set_key_validity_period(blueprint.validity.or(self.primary.validity))?;
 
             if flags.for_certification() || flags.for_signing() {
                 // We need to create a primary key binding signature.
@@ -1070,7 +1070,7 @@ impl CertBuilder {
             .set_features(&Features::sequoia())?
             .set_key_flags(&self.primary.flags)?
             .set_signature_creation_time(creation_time)?
-            .set_key_expiration_time(&key, self.primary.expiration)?
+            .set_key_validity_period(self.primary.validity)?
             .set_preferred_hash_algorithms(vec![
                 HashAlgorithm::SHA512
             ])?
@@ -1264,9 +1264,9 @@ mod tests {
 
         let (cert,_) = CertBuilder::new()
             .set_creation_time(now)
-            .set_expiration_time(now + 600 * s)
+            .set_validity_period(600 * s)
             .add_subkey(KeyFlags::default().set_signing(),
-                        now + 300 * s, None)
+                        300 * s, None)
             .add_subkey(KeyFlags::default().set_authentication(),
                         None, None)
             .generate().unwrap();
diff --git a/openpgp/src/cert/mod.rs b/openpgp/src/cert/mod.rs
index efdc0268..f932292a 100644
--- a/openpgp/src/cert/mod.rs
+++ b/openpgp/src/cert/mod.rs
@@ -2749,13 +2749,15 @@ impl<'a> ValidCert<'a> {
     ///
     /// let creation_time = time::SystemTime::now();
     /// let before_creation = creation_time - a_second;
-    /// let expiration_time = creation_time + 60 * a_second;
+    /// let validity_period = 60 * a_second;
+    /// let expiration_time = creation_time + validity_period;
     /// let before_expiration_time = expiration_time - a_second;
     /// let after_expiration_time = expiration_time + a_second;
     ///
     /// let (cert, _) = CertBuilder::new()
     ///     .add_userid("Alice")
-    ///     .set_expiration_time(expiration_time)
+    ///     .set_creation_time(creation_time)
+    ///     .set_validity_period(validity_period)
     ///     .generate()?;
     ///
     /// // There is no binding signature before the certificate was created.
@@ -3732,14 +3734,12 @@ mod test {
 
     #[test]
     fn set_validity_period_uidless() {
-        use crate::types::{Duration, Timestamp};
+        use crate::types::Duration;
         let p = &P::new();
 
         let (cert, _) = CertBuilder::new()
-            .set_expiration_time(None) // Just to assert this works.
-            .set_expiration_time(
-                Some(Timestamp::now().checked_add(
-                    Duration::weeks(52).unwrap()).unwrap().into()))
+            .set_validity_period(None) // Just to assert this works.
+            .set_validity_period(Some(Duration::weeks(52).unwrap().try_into().unwrap()))
             .generate().unwrap();
         assert_eq!(cert.clone().into_packet_pile().children().count(),
                    1 // primary key
diff --git a/openpgp/src/types/timestamp.rs b/openpgp/src/types/timestamp.rs
index 3a9a0e4c..59b903d1 100644
--- a/openpgp/src/types/timestamp.rs
+++ b/openpgp/src/types/timestamp.rs
@@ -267,11 +267,11 @@ impl Arbitrary for Timestamp {
 /// let p = &StandardPolicy::new();
 ///
 /// let now = Timestamp::now();
-/// let then = now.checked_add(Duration::days(365)?).unwrap();
+/// let validity_period = Duration::days(365)?;
 ///
 /// let (cert,_) = CertBuilder::new()
 ///     .set_creation_time(now)
-///     .set_expiration_time(then)
+///     .set_validity_period(validity_period)
 ///     .generate()?;
 ///
 /// let vc = cert.with_policy(p, now)?;
diff --git a/tool/src/commands/key.rs b/tool/src/commands/key.rs
index abd95433..dc78866f 100644
--- a/tool/src/commands/key.rs
+++ b/tool/src/commands/key.rs
@@ -33,22 +33,24 @@ pub fn generate(m: &ArgMatches, force: bool) -> Result<()> {
     // Expiration.
     match (m.value_of("expires"), m.value_of("expires-in")) {
         (None, None) => // Default expiration.
-            builder = builder.set_expiration_time(
-                Some(SystemTime::now()
-                     + Duration::new(3 * SECONDS_IN_YEAR, 0))),
+            builder = builder.set_validity_period(
+                Some(Duration::new(3 * SECONDS_IN_YEAR, 0))),
         (Some(t), None) if t == "never" =>
-            builder = builder.set_expiration_time(None),
+            builder = builder.set_validity_period(None),
         (Some(t), None) => {
-            let t =
-                crate::parse_iso8601(t, chrono::NaiveTime::from_hms(0, 0, 0))?;
-            builder = builder.set_expiration_time(Some(t.into()));
+            let now = builder.creation_time()
+                .unwrap_or_else(std::time::SystemTime::now);
+            let expiration = SystemTime::from(
+                crate::parse_iso8601(t, chrono::NaiveTime::from_hms(0, 0, 0))?);
+            let validity = expiration.duration_since(now)?;
+            builder = builder.set_creation_time(now)
+                .set_validity_period(validity);
         },
         (None, Some(d)) if d == "never" =>
-            builder = builder.set_expiration_time(None),
+            builder = builder.set_validity_period(None),
         (None, Some(d)) => {
             let d = parse_duration(d)?;
-            builder = builder.set_expiration_time(
-                Some(SystemTime::now() + d));
+            builder = builder.set_validity_period(Some(d));
         },
         (Some(_), Some(_)) => unreachable!("conflicting args"),
     }