openpgp: Reimplement the KeyServerPreferences struct using Bitfield.

  - Also improve the documentation of the KSP::no_modify and the
    corresponding setters.

  - See #525.

3 files changed, 211 insertions, 98 deletions

diff --git a/openpgp/src/packet/signature/subpacket.rs b/openpgp/src/packet/signature/subpacket.rs
index 3055471c..6206c42f 100644
--- a/openpgp/src/packet/signature/subpacket.rs
+++ b/openpgp/src/packet/signature/subpacket.rs
@@ -2659,7 +2659,7 @@ fn accessors() {
     assert_eq!(sig_.preferred_compression_algorithms(), Some(&pref[..]));
 
     let pref = KeyServerPreferences::default()
-        .set_no_modify(true);
+        .set_no_modify();
     sig = sig.set_key_server_preferences(pref.clone()).unwrap();
     let sig_ =
         sig.clone().sign_hash(&mut keypair, hash.clone()).unwrap();
@@ -2962,13 +2962,13 @@ fn subpacket_test_2() {
                        )}));
 
         assert_eq!(sig.key_server_preferences().unwrap(),
-                   KeyServerPreferences::default().set_no_modify(true));
+                   KeyServerPreferences::default().set_no_modify());
         assert_eq!(sig.subpacket(SubpacketTag::KeyServerPreferences),
                    Some(&Subpacket {
                        length: 2.into(),
                        critical: false,
                        value: SubpacketValue::KeyServerPreferences(
-                           KeyServerPreferences::default().set_no_modify(true)),
+                           KeyServerPreferences::default().set_no_modify()),
                    }));
 
         assert!(sig.key_flags().unwrap().for_certification());
diff --git a/openpgp/src/serialize.rs b/openpgp/src/serialize.rs
index c220ebcc..e243e906 100644
--- a/openpgp/src/serialize.rs
+++ b/openpgp/src/serialize.rs
@@ -1388,7 +1388,7 @@ impl Marshal for SubpacketValue {
                     o.write_all(&[(*a).into()])?;
                 },
             KeyServerPreferences(ref p) =>
-                o.write_all(&p.to_vec())?,
+                o.write_all(p.as_slice())?,
             PreferredKeyServer(ref p) =>
                 o.write_all(p)?,
             PrimaryUserID(p) =>
@@ -1455,7 +1455,7 @@ impl MarshalInto for SubpacketValue {
             NotationData(nd) => 4 + 2 + 2 + nd.name().len() + nd.value().len(),
             PreferredHashAlgorithms(ref p) => p.len(),
             PreferredCompressionAlgorithms(ref p) => p.len(),
-            KeyServerPreferences(ref p) => p.to_vec().len(),
+            KeyServerPreferences(ref p) => p.as_slice().len(),
             PreferredKeyServer(ref p) => p.len(),
             PrimaryUserID(_) => 1,
             PolicyURI(ref p) => p.len(),
diff --git a/openpgp/src/types/server_preferences.rs b/openpgp/src/types/server_preferences.rs
index cf4d5a75..fd462c05 100644
--- a/openpgp/src/types/server_preferences.rs
+++ b/openpgp/src/types/server_preferences.rs
@@ -1,9 +1,10 @@
-use std::hash::{Hash, Hasher};
 use std::fmt;
 
 #[cfg(any(test, feature = "quickcheck"))]
 use quickcheck::{Arbitrary, Gen};
 
+use crate::types::Bitfield;
+
 /// Describes preferences regarding key servers.
 ///
 /// Key server preferences are specified in [Section 5.2.3.17 of RFC 4880] and
@@ -12,9 +13,19 @@ use quickcheck::{Arbitrary, Gen};
 /// [Section 5.2.3.17 of RFC 4880]: https://tools.ietf.org/html/rfc4880#section-5.2.3.17
 /// [Section 5.2.3.18 of RFC 4880bis]: https://tools.ietf.org/html/draft-ietf-openpgp-rfc4880bis-09#section-5.2.3.18
 ///
+/// The keyserver preferences are set by the user's OpenPGP
+/// implementation to communicate them to any peers.
+///
 /// # A note on equality
 ///
-/// `PartialEq` implements semantic equality, i.e. it ignores padding.
+/// `PartialEq` compares the serialized form of the two key server
+/// preference sets.  If you prefer to compare two key server
+/// preference sets for semantic equality, you should use
+/// [`KeyServerPreferences::normalized_eq`].  The difference between
+/// semantic equality and serialized equality is that semantic
+/// equality ignores differences in the amount of padding.
+///
+///   [`KeyServerPreferences::normalized_eq`]: #method.normalized_eq
 ///
 /// # Examples
 ///
@@ -43,13 +54,8 @@ use quickcheck::{Arbitrary, Gen};
 /// }
 /// # Ok(()) }
 /// ```
-#[derive(Clone)]
-pub struct KeyServerPreferences{
-    no_modify: bool,
-    unknown: Box<[u8]>,
-    /// Original length, including trailing zeros.
-    pad_to: usize,
-}
+#[derive(Clone, PartialEq, Eq, Hash)]
+pub struct KeyServerPreferences(Bitfield);
 
 impl Default for KeyServerPreferences {
     fn default() -> Self {
@@ -59,112 +65,214 @@ impl Default for KeyServerPreferences {
 
 impl fmt::Debug for KeyServerPreferences {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        let mut dirty = false;
+        let mut need_comma = false;
         if self.no_modify() {
             f.write_str("no modify")?;
-            dirty = true;
+            need_comma = true;
         }
-        if ! self.unknown.is_empty() {
-            if dirty { f.write_str(", ")?; }
-            f.write_str("+0x")?;
-            f.write_str(
-                &crate::fmt::hex::encode_pretty(&self.unknown))?;
-            dirty = true;
+
+        for i in self.0.iter() {
+            match i {
+                KEYSERVER_PREFERENCE_NO_MODIFY => (),
+                i => {
+                    if need_comma { f.write_str(", ")?; }
+                    write!(f, "#{}", i)?;
+                    need_comma = true;
+                },
+            }
         }
-        if self.pad_to >
-            KEYSERVER_PREFERENCES_N_KNOWN_BYTES + self.unknown.len()
-        {
-            if dirty { f.write_str(", ")?; }
-            write!(f, "+padding({} bytes)", self.pad_to - self.unknown.len())?;
+
+        // Mention any padding, as equality is sensitive to this.
+        let padding = self.0.padding_len();
+        if padding > 0 {
+            if need_comma { f.write_str(", ")?; }
+            write!(f, "+padding({} bytes)", padding)?;
         }
 
         Ok(())
     }
 }
 
-impl PartialEq for KeyServerPreferences {
-    fn eq(&self, other: &Self) -> bool {
-        self.no_modify == other.no_modify
-            && self.unknown == other.unknown
-    }
-}
-
-impl Eq for KeyServerPreferences {}
-
-impl Hash for KeyServerPreferences {
-    fn hash<H: Hasher>(&self, state: &mut H) {
-        self.no_modify.hash(state);
-        self.unknown.hash(state);
-    }
-}
-
 impl KeyServerPreferences {
     /// Creates a new instance from `bits`.
     pub fn new<B: AsRef<[u8]>>(bits: B) -> Self {
-        let bits = bits.as_ref();
-        let mut pad_to = 0;
-
-        let no_mod = bits.get(0)
-            .map(|x| x & KEYSERVER_PREFERENCE_NO_MODIFY != 0).unwrap_or(false);
-        let unk = if bits.is_empty() {
-            Box::default()
-        } else {
-            let mut cpy = Vec::from(bits);
-
-            cpy[0] &= KEYSERVER_PREFERENCE_NO_MODIFY ^ 0xff;
+        KeyServerPreferences(bits.as_ref().to_vec().into())
+    }
 
-            pad_to = crate::types::bitfield_remove_padding(&mut cpy);
-            cpy.into_boxed_slice()
-        };
+    /// Returns a slice containing the raw values.
+    pub(crate) fn as_slice(&self) -> &[u8] {
+        self.0.as_slice()
+    }
 
-        KeyServerPreferences{
-            no_modify: no_mod, unknown: unk, pad_to,
-        }
+    /// Compares two key server preference sets for semantic equality.
+    ///
+    /// `KeyServerPreferences`' implementation of `PartialEq` compares
+    /// two key server preference sets for serialized equality.  That
+    /// is, the `PartialEq` implementation considers two key server
+    /// preference sets to *not* be equal if they have different
+    /// amounts of padding.  This comparison function ignores padding.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use sequoia_openpgp as openpgp;
+    /// use openpgp::types::KeyServerPreferences;
+    ///
+    /// # fn main() -> openpgp::Result<()> {
+    /// let a = KeyServerPreferences::new(&[ 0x1 ]);
+    /// let b = KeyServerPreferences::new(&[ 0x1, 0x0 ]);
+    ///
+    /// assert!(a != b);
+    /// assert!(a.normalized_eq(&b));
+    /// # Ok(()) }
+    /// ```
+    pub fn normalized_eq(&self, other: &Self) -> bool {
+        self.0.normalized_eq(&other.0)
     }
 
-    /// Returns a slice referencing the raw values.
-    pub(crate) fn to_vec(&self) -> Vec<u8> {
-        let mut ret = if self.unknown.is_empty() {
-            vec![0]
-        } else {
-            self.unknown.clone().into()
-        };
-
-        if self.no_modify { ret[0] |= KEYSERVER_PREFERENCE_NO_MODIFY; }
-
-        // Corner case: empty flag field.  We initialized ret to
-        // vec![0] for easy setting of flags.  See if any of the above
-        // was set.
-        if ret.len() == 1 && ret[0] == 0 {
-            // Nope.  Trim this byte.
-            ret.pop();
-        }
+    /// Returns whether the specified keyserver preference flag is set.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use sequoia_openpgp as openpgp;
+    /// use openpgp::types::KeyServerPreferences;
+    ///
+    /// # fn main() -> openpgp::Result<()> {
+    /// // Keyserver Preferences flags 0 and 2.
+    /// let ksp = KeyServerPreferences::new(&[ 0x5 ]);
+    ///
+    /// assert!(ksp.get(0));
+    /// assert!(! ksp.get(1));
+    /// assert!(ksp.get(2));
+    /// assert!(! ksp.get(3));
+    /// assert!(! ksp.get(8));
+    /// assert!(! ksp.get(80));
+    /// # assert!(! ksp.no_modify());
+    /// # Ok(()) }
+    /// ```
+    pub fn get(&self, bit: usize) -> bool {
+        self.0.get(bit)
+    }
 
-        for _ in ret.len()..self.pad_to {
-            ret.push(0);
-        }
+    /// Sets the specified keyserver preference flag.
+    ///
+    /// This also clears any padding (trailing NUL bytes).
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use sequoia_openpgp as openpgp;
+    /// use openpgp::types::KeyServerPreferences;
+    ///
+    /// # fn main() -> openpgp::Result<()> {
+    /// let ksp = KeyServerPreferences::default().set(0).set(2);
+    ///
+    /// assert!(ksp.get(0));
+    /// assert!(! ksp.get(1));
+    /// assert!(ksp.get(2));
+    /// assert!(! ksp.get(3));
+    /// # assert!(! ksp.no_modify());
+    /// # Ok(()) }
+    /// ```
+    pub fn set(self, bit: usize) -> Self {
+        Self(self.0.set(bit))
+    }
 
-        ret
+    /// Clears the specified keyserver preference flag.
+    ///
+    /// This also clears any padding (trailing NUL bytes).
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use sequoia_openpgp as openpgp;
+    /// use openpgp::types::KeyServerPreferences;
+    ///
+    /// # fn main() -> openpgp::Result<()> {
+    /// let ksp = KeyServerPreferences::default().set(0).set(2).clear(2);
+    ///
+    /// assert!(ksp.get(0));
+    /// assert!(! ksp.get(1));
+    /// assert!(! ksp.get(2));
+    /// assert!(! ksp.get(3));
+    /// # assert!(! ksp.no_modify());
+    /// # Ok(()) }
+    /// ```
+    pub fn clear(self, bit: usize) -> Self {
+        Self(self.0.clear(bit))
     }
 
-    /// Whether or not keyservers are allowed to modify this key.
+    /// Returns whether the certificate's owner requests that the
+    /// certificate is not modified.
+    ///
+    /// If this flag is set, the certificate's owner requests that the
+    /// certificate should only be changed by the owner and the key
+    /// server's operator.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use sequoia_openpgp as openpgp;
+    /// use openpgp::types::KeyServerPreferences;
+    ///
+    /// # fn main() -> openpgp::Result<()> {
+    /// let ksp = KeyServerPreferences::default();
+    /// assert!(! ksp.no_modify());
+    /// # Ok(()) }
+    /// ```
     pub fn no_modify(&self) -> bool {
-        self.no_modify
+        self.get(KEYSERVER_PREFERENCE_NO_MODIFY)
     }
 
-    /// Sets whether or not keyservers are allowed to modify this key.
-    pub fn set_no_modify(mut self, v: bool) -> Self {
-        self.no_modify = v;
-        self
+    /// Requests that the certificate is not modified.
+    ///
+    /// See [`KeyServerPreferences::no_modify`].
+    ///
+    ///   [`KeyServerPreferences::no_modify`]: #method.no_modify
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use sequoia_openpgp as openpgp;
+    /// use openpgp::types::KeyServerPreferences;
+    ///
+    /// # fn main() -> openpgp::Result<()> {
+    /// let ksp = KeyServerPreferences::default().set_no_modify();
+    /// assert!(ksp.no_modify());
+    /// # Ok(()) }
+    /// ```
+    pub fn set_no_modify(self) -> Self {
+        self.set(KEYSERVER_PREFERENCE_NO_MODIFY)
     }
-}
 
-/// The private component of this key may be in the possession of more
-/// than one person.
-const KEYSERVER_PREFERENCE_NO_MODIFY: u8 = 0x80;
+    /// Clears the request that the certificate is not modified.
+    ///
+    /// See [`KeyServerPreferences::no_modify`].
+    ///
+    ///   [`KeyServerPreferences::no_modify`]: #method.no_modify
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use sequoia_openpgp as openpgp;
+    /// use openpgp::types::KeyServerPreferences;
+    ///
+    /// # fn main() -> openpgp::Result<()> {
+    /// let ksp = KeyServerPreferences::new(&[0x80][..]);
+    /// assert!(ksp.no_modify());
+    /// let ksp = ksp.clear_no_modify();
+    /// assert!(! ksp.no_modify());
+    /// # Ok(()) }
+    /// ```
+    pub fn clear_no_modify(self) -> Self {
+        self.clear(KEYSERVER_PREFERENCE_NO_MODIFY)
+    }
+}
 
-/// Number of bytes with known flags.
-const KEYSERVER_PREFERENCES_N_KNOWN_BYTES: usize = 1;
+/// The key holder requests that this key only be modified or updated
+/// by the key holder or an administrator of the key server.
+const KEYSERVER_PREFERENCE_NO_MODIFY: usize = 7;
 
 #[cfg(any(test, feature = "quickcheck"))]
 impl Arbitrary for KeyServerPreferences {
@@ -190,13 +298,18 @@ mod tests {
 
     quickcheck! {
         fn roundtrip(val: KeyServerPreferences) -> bool {
-            let q = KeyServerPreferences::new(&val.to_vec());
+            let mut q = KeyServerPreferences::new(val.as_slice());
             assert_eq!(val, q);
+            assert!(val.normalized_eq(&q));
+
+            // Add some padding to q.  Make sure they are still equal.
+            q.0.raw.push(0);
+            assert!(val != q);
+            assert!(val.normalized_eq(&q));
 
-            // Check that equality ignores padding.
-            let mut val_without_padding = val.clone();
-            val_without_padding.pad_to = val.unknown.len();
-            assert_eq!(val, val_without_padding);
+            q.0.raw.push(0);
+            assert!(val != q);
+            assert!(val.normalized_eq(&q));
 
             true
         }