Implement PartialEq and Eq for algorithm types #553

rodentrabies · 2017-06-23T07:46:45Z

PR for #542

#[derive(Eq)] seems to not work for algorithm types because of other fields (with fn types for example), so explicit impl Eq for Type {} were added.

Also, some unintended diff in aead/chacha20_poly1305.rs and agreement.rs, because I use rustfmt tool in my emacs. If this is a problem, I will restore it.

coveralls · 2017-06-23T11:33:16Z

Coverage decreased (-0.3%) to 95.575% when pulling 9ec19b6dd827608b80336b71a4a7aaf9bd770d59 on mrwhythat:algorithms-eq-implementation into 67c526b on briansmith:master.

briansmith

Thanks!

briansmith · 2017-06-29T21:22:49Z

src/aead/aead.rs

+    fn eq(&self, other: &Self) -> bool { self.id == other.id }
+}
+
+impl Eq for Algorithm {}


I think Eq can be derived and only PartialEq needs to be explicitly implemented, right? Assuming so, let's always derive Eq instead of doing impl Eq for ....

Well, I tried deriving Eq first, but it broke the build. It seems that non-Eq fields make it impossible to derive. Or maybe it's the ordering. I'll research.

Did some research and it seems that derive(Eq) simply checks for every field, it does not look for existing required impls, like Haskell does.

I looked into it. It used to work, but now it doesn't, and that is intentional: rust-lang/rust#36830.

briansmith · 2017-06-29T21:23:39Z

src/aead/chacha20_poly1305.rs

 };

 /// Copies |key| into |ctx_buf|.
 pub fn chacha20_poly1305_init(ctx_buf: &mut [u8], key: &[u8])
-                              -> Result<(), error::Unspecified> {
+    -> Result<(), error::Unspecified> {


Probably we should reformat the code, but not in this PR. Please remove all the unrelated formatting changes.

briansmith · 2017-06-29T21:24:30Z

src/agreement.rs

+    ECDH_P256,
+    /// Defined in 'ec/suite_b/ecdh.rs'
+    ECDH_P384,
+    /// Defined in 'ec/curve25519/x25519.rs'


Let's drop all the "Defined in" comments.

briansmith · 2017-06-29T21:28:34Z

src/agreement.rs

+
+/// Key agreement algorithm types.
+#[derive(PartialEq, Eq)]
+#[allow(non_camel_case_types)]


Let's put the allow(...) ahead of derive(...) in this and the other ones. No order seems better than any other, so let's things in alphabetical order so we don't have to decide another order.

briansmith · 2017-06-29T21:32:42Z

src/ec/suite_b/ecdsa.rs

@@ -50,6 +66,12 @@ enum ECDSAVerificationAlgorithmID {
    ECDSA_P384_SHA384_FIXED,
 }

+impl PartialEq for ECDSAVerificationAlgorithm {
+    fn eq(&self, other: &Self) -> bool { self.id == other.id }
+}


I think we should not implement PartialEq for this type. Instead we should implement PartialEq for signature::VerificationAlgorithm in some way. Maybe we need to make signature::VerificationAlgorithm a struct that can hold the id field first. WDYT?

Well, there is PartialEq implementation for ECDSASigningAlgorithm, so implementing it for ECDSAVerificationAlgorithm only benefits the symmetricity. Also, signature::VerificationAlgorithm is a very elegant single-method protocol, which I'm a big fan of, and which modern cryptographic libraries lack, so I think coercing it to struct is a bad idea, but it may just be my irrational thinking:)

Well, there is PartialEq implementation for ECDSASigningAlgorithm

Yes, but that's because we've not yet found a solution for creating a unifying ring::signature::SigningAlgorithm interface.

However, we already have ring::signature::VerificationAlgorithm, and ultimatenly for these comparison operators to be most useful, we need to be able to compare two VerificationAlgorithms, including comparing an RSA algorithm to an ECDSA algorithm. That means that ring::signature::VerificationAlgorithm is what ultimately needs to implement Eq/PartialEq. Let's try to find a way to do that. For now, I'll remove this part.

rodentrabies · 2017-06-30T20:52:54Z

@briansmith , just committed requested changes

briansmith · 2017-07-07T03:52:51Z

src/aead/aead.rs

+    fn eq(&self, other: &Self) -> bool { self.id == other.id }
+}
+
+impl Eq for Algorithm {}


I looked into it. It used to work, but now it doesn't, and that is intentional: rust-lang/rust#36830.

briansmith · 2017-07-07T03:53:30Z

src/agreement.rs

+pub enum AlgorithmID {
+    ECDH_P256,
+    ECDH_P384,
+    X25519,
 }


CurveID already exists, so we don't need this. I'll remove this in favor of an approach using CurveID.

Removed it in favor of algorithm.i.curve.id comparison.

briansmith · 2017-07-07T03:59:57Z

src/ec/suite_b/ecdsa.rs

@@ -50,6 +66,12 @@ enum ECDSAVerificationAlgorithmID {
    ECDSA_P384_SHA384_FIXED,
 }

+impl PartialEq for ECDSAVerificationAlgorithm {
+    fn eq(&self, other: &Self) -> bool { self.id == other.id }
+}


Well, there is PartialEq implementation for ECDSASigningAlgorithm

Yes, but that's because we've not yet found a solution for creating a unifying ring::signature::SigningAlgorithm interface.

However, we already have ring::signature::VerificationAlgorithm, and ultimatenly for these comparison operators to be most useful, we need to be able to compare two VerificationAlgorithms, including comparing an RSA algorithm to an ECDSA algorithm. That means that ring::signature::VerificationAlgorithm is what ultimately needs to implement Eq/PartialEq. Let's try to find a way to do that. For now, I'll remove this part.

I agree to license my contributions to each file under the terms given at the top of each file I changed.

coveralls · 2017-07-25T01:23:34Z

Coverage decreased (-0.08%) to 95.596% when pulling 0fdb9b4 on mrwhythat:algorithms-eq-implementation into 5f8d332 on briansmith:master.

rodentrabies · 2017-08-20T13:10:12Z

Any additional changes required?

ctz · 2017-08-20T15:00:24Z

This will let me delete some code that computes digest::Algorithm equality the dumb way 👍

briansmith · 2017-09-04T00:47:08Z

This landed with modifications and was released in ring 0.12.1. The commits are:

d8134d6
eb668d5
b3cd49c

Thanks for submitting this and sorry for the long delay in getting it landed.

@ctz wrote:

This will let me delete some code that computes digest::Algorithm equality the dumb way 👍

I just noticed that, and just sent you a PR: rustls/rustls#117

rodentrabies mentioned this pull request Jun 23, 2017

Implement PartialEq and Eq for all public Algorithm types #542

Closed

briansmith requested changes Jun 29, 2017

View reviewed changes

rodentrabies force-pushed the algorithms-eq-implementation branch from 9ec19b6 to 89bef08 Compare June 30, 2017 20:51

briansmith reviewed Jul 7, 2017

View reviewed changes

rodentrabies force-pushed the algorithms-eq-implementation branch from 89bef08 to e62a72d Compare July 24, 2017 21:17

Implement PartialEq and Eq for agorithm types

0fdb9b4

I agree to license my contributions to each file under the terms given at the top of each file I changed.

rodentrabies force-pushed the algorithms-eq-implementation branch from e62a72d to 0fdb9b4 Compare July 24, 2017 21:19

rodentrabies changed the title ~~Implement PartialEq and Eq for agorithm types~~ Implement PartialEq and Eq for algorithm types Aug 25, 2017

briansmith closed this Sep 4, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement PartialEq and Eq for algorithm types #553

Implement PartialEq and Eq for algorithm types #553

rodentrabies commented Jun 23, 2017

coveralls commented Jun 23, 2017 •

edited

Loading

briansmith left a comment

briansmith Jun 29, 2017

rodentrabies Jun 30, 2017

rodentrabies Jun 30, 2017

briansmith Jul 7, 2017

briansmith Jun 29, 2017

briansmith Jun 29, 2017

briansmith Jun 29, 2017

briansmith Jun 29, 2017

rodentrabies Jun 30, 2017

briansmith Jul 7, 2017

rodentrabies commented Jun 30, 2017

briansmith Jul 7, 2017

briansmith Jul 7, 2017

rodentrabies Jul 24, 2017

briansmith Jul 7, 2017

coveralls commented Jul 25, 2017 •

edited

Loading

rodentrabies commented Aug 20, 2017 •

edited

Loading

ctz commented Aug 20, 2017

briansmith commented Sep 4, 2017

Implement PartialEq and Eq for algorithm types #553

Implement PartialEq and Eq for algorithm types #553

Conversation

rodentrabies commented Jun 23, 2017

coveralls commented Jun 23, 2017 • edited Loading

briansmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodentrabies commented Jun 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 25, 2017 • edited Loading

rodentrabies commented Aug 20, 2017 • edited Loading

ctz commented Aug 20, 2017

briansmith commented Sep 4, 2017

coveralls commented Jun 23, 2017 •

edited

Loading

coveralls commented Jul 25, 2017 •

edited

Loading

rodentrabies commented Aug 20, 2017 •

edited

Loading