Merge pull request #227 from erickt/keyid-order
Preserve the order of key ids in the root metadata
diff --git a/src/interchange/cjson/shims.rs b/src/interchange/cjson/shims.rs
index 84bf28e..d916318 100644
--- a/src/interchange/cjson/shims.rs
+++ b/src/interchange/cjson/shims.rs
@@ -107,12 +107,11 @@
impl RoleDefinition {
pub fn from(role: &metadata::RoleDefinition) -> Result<Self> {
- let mut key_ids = role
+ let key_ids = role
.key_ids()
.iter()
.cloned()
.collect::<Vec<crypto::KeyId>>();
- key_ids.sort();
Ok(RoleDefinition {
threshold: role.threshold(),
@@ -120,7 +119,7 @@
})
}
- pub fn try_into(mut self) -> Result<metadata::RoleDefinition> {
+ pub fn try_into(self) -> Result<metadata::RoleDefinition> {
let vec_len = self.key_ids.len();
if vec_len < 1 {
return Err(Error::Encoding(
@@ -128,8 +127,13 @@
));
}
- let key_ids = self.key_ids.drain(0..).collect::<HashSet<crypto::KeyId>>();
- let dupes = vec_len - key_ids.len();
+ let mut seen = HashSet::new();
+ let mut dupes = 0;
+ for key_id in self.key_ids.iter() {
+ if !seen.insert(key_id) {
+ dupes += 1;
+ }
+ }
if dupes != 0 {
return Err(Error::Encoding(format!(
@@ -138,7 +142,7 @@
)));
}
- Ok(metadata::RoleDefinition::new(self.threshold, key_ids)?)
+ Ok(metadata::RoleDefinition::new(self.threshold, self.key_ids)?)
}
}
diff --git a/src/metadata.rs b/src/metadata.rs
index 3c7d72c..c2db2ef 100644
--- a/src/metadata.rs
+++ b/src/metadata.rs
@@ -489,13 +489,13 @@
consistent_snapshot: bool,
keys: HashMap<KeyId, PublicKey>,
root_threshold: u32,
- root_key_ids: HashSet<KeyId>,
+ root_key_ids: Vec<KeyId>,
snapshot_threshold: u32,
- snapshot_key_ids: HashSet<KeyId>,
+ snapshot_key_ids: Vec<KeyId>,
targets_threshold: u32,
- targets_key_ids: HashSet<KeyId>,
+ targets_key_ids: Vec<KeyId>,
timestamp_threshold: u32,
- timestamp_key_ids: HashSet<KeyId>,
+ timestamp_key_ids: Vec<KeyId>,
}
impl RootMetadataBuilder {
@@ -512,13 +512,13 @@
consistent_snapshot: false,
keys: HashMap::new(),
root_threshold: 1,
- root_key_ids: HashSet::new(),
+ root_key_ids: Vec::new(),
snapshot_threshold: 1,
- snapshot_key_ids: HashSet::new(),
+ snapshot_key_ids: Vec::new(),
targets_threshold: 1,
- targets_key_ids: HashSet::new(),
+ targets_key_ids: Vec::new(),
timestamp_threshold: 1,
- timestamp_key_ids: HashSet::new(),
+ timestamp_key_ids: Vec::new(),
}
}
@@ -550,7 +550,7 @@
pub fn root_key(mut self, public_key: PublicKey) -> Self {
let key_id = public_key.key_id().clone();
self.keys.insert(key_id.clone(), public_key);
- self.root_key_ids.insert(key_id);
+ self.root_key_ids.push(key_id);
self
}
@@ -564,7 +564,7 @@
pub fn snapshot_key(mut self, public_key: PublicKey) -> Self {
let key_id = public_key.key_id().clone();
self.keys.insert(key_id.clone(), public_key);
- self.snapshot_key_ids.insert(key_id);
+ self.snapshot_key_ids.push(key_id);
self
}
@@ -578,7 +578,7 @@
pub fn targets_key(mut self, public_key: PublicKey) -> Self {
let key_id = public_key.key_id().clone();
self.keys.insert(key_id.clone(), public_key);
- self.targets_key_ids.insert(key_id);
+ self.targets_key_ids.push(key_id);
self
}
@@ -592,7 +592,7 @@
pub fn timestamp_key(mut self, public_key: PublicKey) -> Self {
let key_id = public_key.key_id().clone();
self.keys.insert(key_id.clone(), public_key);
- self.timestamp_key_ids.insert(key_id);
+ self.timestamp_key_ids.push(key_id);
self
}
@@ -756,12 +756,12 @@
#[derive(Clone, Debug, PartialEq)]
pub struct RoleDefinition {
threshold: u32,
- key_ids: HashSet<KeyId>,
+ key_ids: Vec<KeyId>,
}
impl RoleDefinition {
/// Create a new `RoleDefinition` with a given threshold and set of authorized `KeyID`s.
- pub fn new(threshold: u32, key_ids: HashSet<KeyId>) -> Result<Self> {
+ pub fn new(threshold: u32, key_ids: Vec<KeyId>) -> Result<Self> {
if threshold < 1 {
return Err(Error::IllegalArgument(format!("Threshold: {}", threshold)));
}
@@ -789,7 +789,7 @@
}
/// An immutable reference to the set of `KeyID`s that are authorized to sign the role.
- pub fn key_ids(&self) -> &HashSet<KeyId> {
+ pub fn key_ids(&self) -> &[KeyId] {
&self.key_ids
}
}
@@ -2025,18 +2025,20 @@
#[test]
fn serde_role_definition() {
- let hashes = hashset!(
- "01892c662c8cd79fab20edec21de1dcb8b75d9353103face7fe086ff5c0098e4",
- "4750eaf6878740780d6f97b12dbad079fb012bec88c78de2c380add56d3f51db",
- )
- .iter()
- .map(|k| KeyId::from_str(*k).unwrap())
- .collect();
- let role_def = RoleDefinition::new(2, hashes).unwrap();
+ // keyid ordering must be preserved.
+ let keyids = vec![
+ KeyId::from_str("40e35e8f6003ab90d104710cf88901edab931597401f91c19eeb366060ab3d53")
+ .unwrap(),
+ KeyId::from_str("01892c662c8cd79fab20edec21de1dcb8b75d9353103face7fe086ff5c0098e4")
+ .unwrap(),
+ KeyId::from_str("4750eaf6878740780d6f97b12dbad079fb012bec88c78de2c380add56d3f51db")
+ .unwrap(),
+ ];
+ let role_def = RoleDefinition::new(3, keyids).unwrap();
let jsn = json!({
- "threshold": 2,
+ "threshold": 3,
"keyids": [
- // these need to be sorted for determinism
+ "40e35e8f6003ab90d104710cf88901edab931597401f91c19eeb366060ab3d53",
"01892c662c8cd79fab20edec21de1dcb8b75d9353103face7fe086ff5c0098e4",
"4750eaf6878740780d6f97b12dbad079fb012bec88c78de2c380add56d3f51db",
],
@@ -2045,7 +2047,10 @@
assert_eq!(encoded, jsn);
let decoded: RoleDefinition = serde_json::from_value(encoded).unwrap();
assert_eq!(decoded, role_def);
+ }
+ #[test]
+ fn serde_invalid_role_definitions() {
let jsn = json!({
"threshold": 0,
"keyids": [
@@ -2748,12 +2753,12 @@
fn deserialize_json_role_definition_illegal_threshold() {
let role_def = RoleDefinition::new(
1,
- hashset!(
+ vec![
PrivateKey::from_pkcs8(ED25519_1_PK8, SignatureScheme::Ed25519)
.unwrap()
.key_id()
- .clone()
- ),
+ .clone(),
+ ],
)
.unwrap();
@@ -2767,7 +2772,7 @@
let role_def = RoleDefinition::new(
2,
- hashset!(
+ vec![
PrivateKey::from_pkcs8(ED25519_1_PK8, SignatureScheme::Ed25519)
.unwrap()
.key_id()
@@ -2776,7 +2781,7 @@
.unwrap()
.key_id()
.clone(),
- ),
+ ],
)
.unwrap();
@@ -2814,7 +2819,7 @@
.unwrap()
.key_id()
.clone();
- let role_def = RoleDefinition::new(1, hashset!(key_id.clone())).unwrap();
+ let role_def = RoleDefinition::new(1, vec![key_id.clone()]).unwrap();
let mut jsn = serde_json::to_value(&role_def).unwrap();
match jsn.as_object_mut() {
