Skip to content
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

feat: add constant-time trait bounds #219

Merged
merged 1 commit into from
Feb 7, 2024

Conversation

AaronFeickert
Copy link
Contributor

@AaronFeickert AaronFeickert commented Jan 15, 2024

Currently, the only implementation of the SecretKey and PublicKey traits is for Ristretto, where both scalars and group elements use constant-time equality in their underlying PartialEq implementations, and which support the ConstantTimeEq trait.

This PR does what it can to encourage the use of constant-time equality for keys by doing a few things.

First, it requires that any types implementing SecretKey or PublicKey also implement ConstantTimeEq. Unfortunately, this doesn't guarantee that their PartialEq implementation defaults to this, and it doesn't appear possible to enforce this at the trait level.

It also sets a good example by manually implementing PartialEq on the Ristretto key types to use their ConstantTimeEq implementations. This isn't strictly necessary, but hopefully helps to indicate best practice. It also implements ConstantTimeEq directly as required by the new trait bounds.

Finally, it implements ConstantTimeEq for DiffieHellmanSharedSecret using the new trait bound, and removes a redundant Zeroize trait bound.

Note that this doesn't actually change the current implementations' behavior, and therefore incurs no performance hit.

Closes #139.

@ghpbot-tari-project ghpbot-tari-project added the CR-insufficient_context The PR body does not provide enough information to fully describe the changes requested. label Jan 15, 2024
@AaronFeickert AaronFeickert marked this pull request as ready for review January 15, 2024 22:40
sdbondi
sdbondi previously approved these changes Feb 5, 2024
Copy link
Member

@sdbondi sdbondi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@SWvheerden SWvheerden merged commit a6cef07 into tari-project:main Feb 7, 2024
4 checks passed
@AaronFeickert AaronFeickert deleted the ct_eq branch February 7, 2024 16:45
@AaronFeickert
Copy link
Contributor Author

I have an open PR in subtle to add a marker trait intended to signal that all equality testing for a type is done in constant time. If this is merged and then used in the curve25519-dalek types, it could be used as a trait bound here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CR-insufficient_context The PR body does not provide enough information to fully describe the changes requested. P-acks_required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check that secret-type material uses constant-time comparisons
4 participants