feat: add collision resistance for truncated key id #45
Conversation
src/amortized_tokens/server.rs
Outdated
| if key_store.get(&truncated_token_key_id).await.is_some() { | ||
| continue; | ||
| } | ||
|
|
||
| key_store.insert(truncated_token_key_id, server).await; |
There was a problem hiding this comment.
I don't think it's critical (like, could just document it), but this is a Time-of-Check-Time-of-Use race condition, and a fix would be to write an atomic insert if absent method on the key_stores.
src/amortized_tokens/server.rs
Outdated
| OsRng.fill_bytes(&mut seed); | ||
| self.create_keypair_internal(key_store, &seed, b"PrivacyPass") | ||
| .await | ||
| loop { |
There was a problem hiding this comment.
the other servers have a loop limit, this one does not.
src/private_tokens/server.rs
Outdated
| key_store.insert(truncated_token_key_id, server).await; | ||
| return Ok(public_key); | ||
| } | ||
| Err(CreateKeypairError::SeedError) |
There was a problem hiding this comment.
#[error("Seed is too long")] seems sufficiently incorrect that you might want to make a CollisionExhausted maybe?
src/public_tokens/server.rs
Outdated
| key_store | ||
| .insert(truncated_token_key_id, key_pair.clone()) | ||
| .await; | ||
| return Ok(key_pair.pk); |
There was a problem hiding this comment.
Instead of cloning the privkey as well, we can just clone out the pubkey
| key_store | |
| .insert(truncated_token_key_id, key_pair.clone()) | |
| .await; | |
| return Ok(key_pair.pk); | |
| let public_key = key_pair.pk.clone(); | |
| key_store | |
| .insert(truncated_token_key_id, key_pair) | |
| .await; | |
| return Ok(public_key); |
src/private_tokens/server.rs
Outdated
| let server = Self::server_from_seed(&seed, b"PrivacyPass")?; | ||
| let public_key = server.get_public_key(); | ||
| let truncated_token_key_id = | ||
| truncate_token_key_id(&public_key_to_token_key_id::<CS>(&server.get_public_key())); |
There was a problem hiding this comment.
| truncate_token_key_id(&public_key_to_token_key_id::<CS>(&server.get_public_key())); | |
| truncate_token_key_id(&public_key_to_token_key_id::<CS>(&public_key)); |
src/amortized_tokens/server.rs
Outdated
| let server = Self::server_from_seed(&seed, b"PrivacyPass")?; | ||
| let public_key = server.get_public_key(); | ||
| let truncated_token_key_id = | ||
| truncate_token_key_id(&public_key_to_token_key_id::<CS>(&server.get_public_key())); |
There was a problem hiding this comment.
| truncate_token_key_id(&public_key_to_token_key_id::<CS>(&server.get_public_key())); | |
| truncate_token_key_id(&public_key_to_token_key_id::<CS>(&public_key)); |
src/private_tokens/server.rs
Outdated
| self.create_keypair_internal(key_store, &seed, b"PrivacyPass") | ||
| .await | ||
| } | ||
| let attempts_limit = 100; |
There was a problem hiding this comment.
maybe make this a constant somewhere, and use the same one for all three servers?
src/private_tokens/server.rs
Outdated
| } | ||
| let attempts_limit = 100; | ||
| for _ in 0..attempts_limit { | ||
| let mut seed = vec![0u8; <<CS::Group as Group>::ScalarLen as Unsigned>::USIZE]; |
There was a problem hiding this comment.
You use GenericArray in Amortized Tokens, which puts the seed on the stack. That seems good, but I'd suggest just doing the same thing everywhere, whichever you like.
src/public_tokens/server.rs
Outdated
| let mut verified = false; | ||
| for public_key in public_keys { | ||
| if signature | ||
| .verify(&public_key, None, token_input_bytes.clone(), &options) | ||
| .is_ok() | ||
| { | ||
| verified = true; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Maybe this is an opportunity to use iter().any()? Something like:
| let mut verified = false; | |
| for public_key in public_keys { | |
| if signature | |
| .verify(&public_key, None, token_input_bytes.clone(), &options) | |
| .is_ok() | |
| { | |
| verified = true; | |
| break; | |
| } | |
| } | |
| let verified = public_keys.iter().any(|public_key| { | |
| signature.verify(public_key, None, token_input_bytes.clone(), &options).is_ok() | |
| }); |
perhaps we can get rid of the .clone() on each iteration, too? hmmm
|
Thanks for the review, @jcjones. Much appreciated! |
Fixes #43.