-
Notifications
You must be signed in to change notification settings - Fork 13
feat: add collision resistance for truncated key id #45
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
c9402b3
e1c575f
0851a2e
4ef03d9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -25,6 +25,14 @@ pub struct Server<CS: PrivateCipherSuite> { | |||||
| } | ||||||
|
|
||||||
| impl<CS: PrivateCipherSuite> Server<CS> { | ||||||
| fn server_from_seed(seed: &[u8], info: &[u8]) -> Result<VoprfServer<CS>, CreateKeypairError> | ||||||
| where | ||||||
| <CS::Group as Group>::Scalar: Send + Sync, | ||||||
| <CS::Group as Group>::Elem: Send + Sync, | ||||||
| { | ||||||
| VoprfServer::<CS>::new_from_seed(seed, info).map_err(|_| CreateKeypairError::SeedError) | ||||||
| } | ||||||
|
|
||||||
| /// Create a new server. The new server does not contain any key material. | ||||||
| #[must_use] | ||||||
| pub const fn new() -> Self { | ||||||
|
|
@@ -45,14 +53,27 @@ impl<CS: PrivateCipherSuite> Server<CS> { | |||||
| <CS::Group as Group>::Scalar: Send + Sync, | ||||||
| <CS::Group as Group>::Elem: Send + Sync, | ||||||
| { | ||||||
| let mut seed = GenericArray::<_, <CS::Group as Group>::ScalarLen>::default(); | ||||||
| OsRng.fill_bytes(&mut seed); | ||||||
| self.create_keypair_internal(key_store, &seed, b"PrivacyPass") | ||||||
| .await | ||||||
| loop { | ||||||
| let mut seed = GenericArray::<_, <CS::Group as Group>::ScalarLen>::default(); | ||||||
| OsRng.fill_bytes(&mut seed); | ||||||
| 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())); | ||||||
|
||||||
| 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)); |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -26,6 +26,10 @@ pub struct Server<CS: PrivateCipherSuite> { | |||||
| } | ||||||
|
|
||||||
| impl<CS: PrivateCipherSuite> Server<CS> { | ||||||
| fn server_from_seed(seed: &[u8], info: &[u8]) -> Result<VoprfServer<CS>, CreateKeypairError> { | ||||||
| VoprfServer::<CS>::new_from_seed(seed, info).map_err(|_| CreateKeypairError::SeedError) | ||||||
| } | ||||||
|
|
||||||
| /// Creates a new server. | ||||||
| #[must_use] | ||||||
| pub const fn new() -> Self { | ||||||
|
|
@@ -42,26 +46,23 @@ impl<CS: PrivateCipherSuite> Server<CS> { | |||||
| &self, | ||||||
| key_store: &PKS, | ||||||
| ) -> Result<PublicKey<CS>, CreateKeypairError> { | ||||||
| let mut seed = vec![0u8; <<CS::Group as Group>::ScalarLen as Unsigned>::USIZE]; | ||||||
| OsRng.fill_bytes(&mut seed); | ||||||
| self.create_keypair_internal(key_store, &seed, b"PrivacyPass") | ||||||
| .await | ||||||
| } | ||||||
| let attempts_limit = 100; | ||||||
|
||||||
| for _ in 0..attempts_limit { | ||||||
| let mut seed = vec![0u8; <<CS::Group as Group>::ScalarLen as Unsigned>::USIZE]; | ||||||
|
||||||
| OsRng.fill_bytes(&mut seed); | ||||||
| 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())); | ||||||
|
||||||
| 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)); |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#[error("Seed is too long")] seems sufficiently incorrect that you might want to make a CollisionExhausted maybe?
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -30,8 +30,8 @@ pub trait IssuerKeyStore: Send + Sync { | |||||||||||||||||||||||||||
| pub trait OriginKeyStore { | ||||||||||||||||||||||||||||
| /// Inserts a keypair with a given `truncated_token_key_id` into the key store. | ||||||||||||||||||||||||||||
| async fn insert(&self, truncated_token_key_id: TruncatedTokenKeyId, server: PublicKey); | ||||||||||||||||||||||||||||
| /// Returns a keypair with a given `truncated_token_key_id` from the key store. | ||||||||||||||||||||||||||||
| async fn get(&self, truncated_token_key_id: &TruncatedTokenKeyId) -> Option<PublicKey>; | ||||||||||||||||||||||||||||
| /// Returns all public keys with a given `truncated_token_key_id` from the key store. | ||||||||||||||||||||||||||||
| async fn get(&self, truncated_token_key_id: &TruncatedTokenKeyId) -> Vec<PublicKey>; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| /// Serializes a keypair into a DER-encoded PKCS#8 document. | ||||||||||||||||||||||||||||
|
|
@@ -64,14 +64,23 @@ impl IssuerServer { | |||||||||||||||||||||||||||
| rng: &mut R, | ||||||||||||||||||||||||||||
| key_store: &IKS, | ||||||||||||||||||||||||||||
| ) -> Result<PublicKey, CreateKeypairError> { | ||||||||||||||||||||||||||||
| let key_pair = | ||||||||||||||||||||||||||||
| KeyPair::generate(rng, KEYSIZE_IN_BITS).map_err(|_| CreateKeypairError::SeedError)?; | ||||||||||||||||||||||||||||
| let truncated_token_key_id = | ||||||||||||||||||||||||||||
| truncate_token_key_id(&public_key_to_token_key_id(&key_pair.pk)); | ||||||||||||||||||||||||||||
| key_store | ||||||||||||||||||||||||||||
| .insert(truncated_token_key_id, key_pair.clone()) | ||||||||||||||||||||||||||||
| .await; | ||||||||||||||||||||||||||||
| Ok(key_pair.pk) | ||||||||||||||||||||||||||||
| let attempts_limit = 100; | ||||||||||||||||||||||||||||
| for _ in 0..attempts_limit { | ||||||||||||||||||||||||||||
| let key_pair = KeyPair::generate(rng, KEYSIZE_IN_BITS) | ||||||||||||||||||||||||||||
| .map_err(|_| CreateKeypairError::SeedError)?; | ||||||||||||||||||||||||||||
| let truncated_token_key_id = | ||||||||||||||||||||||||||||
| truncate_token_key_id(&public_key_to_token_key_id(&key_pair.pk)); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| if key_store.get(&truncated_token_key_id).await.is_some() { | ||||||||||||||||||||||||||||
| continue; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| key_store | ||||||||||||||||||||||||||||
| .insert(truncated_token_key_id, key_pair.clone()) | ||||||||||||||||||||||||||||
| .await; | ||||||||||||||||||||||||||||
| return Ok(key_pair.pk); | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
| 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); |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the other servers have a loop limit, this one does not.