rename "djb" type curve to "curve25519" everywhere #475

cosmicexplorer · 2022-06-27T08:02:33Z

Broken out from #287.

Rename KeyType::Djb to KeyType::Curve25519.
Rename "DJB_TYPE" to "CURVE_25519_TYPE" in java and swift
Add docs and the Keyed trait to curve*.rs

TODO

See this comment: Add docstrings for 7 more Rust API modules #287 (comment).

jrose-signal

I forgot to check which PRs were still drafts, whoops. Many comments on this one, and I feel a little bad for being like this since this is how we go months without any documentation progress.

jrose-signal · 2022-06-29T23:28:44Z

java/shared/java/org/signal/libsignal/protocol/ecc/Curve.java

 *
 * Licensed according to the LICENSE file in this repository.
 */
 package org.signal.libsignal.protocol.ecc;
 import org.signal.libsignal.protocol.InvalidKeyException;

 public class Curve {
-  public  static final int DJB_TYPE   = 0x05;
+  public static final int CURVE_25519_TYPE   = 0x05;


Since the constant here is public, let's leave the old name here too but mark it deprecated. (You can see the combined annotation + javadoc idiom for that elsewhere in the library.)

jrose-signal · 2022-06-29T23:28:54Z

java/shared/java/org/signal/libsignal/protocol/ecc/ECPublicKey.java

@@ -26,7 +26,7 @@ public ECPublicKey(byte[] serialized) {

  static public ECPublicKey fromPublicKeyBytes(byte[] key) {
    byte[] with_type = new byte[33];
-    with_type[0] = 0x05;
+    with_type[0] = Curve.CURVE_25519_TYPE;


Yuck, good find.

jrose-signal · 2022-06-29T23:30:10Z

rust/crypto/src/aes_ctr.rs

+    pub const CTR_NONCE_SIZE: usize = aes::BLOCK_SIZE - 4;
+
+    const NONCE_SIZE: usize = Self::CTR_NONCE_SIZE;


Please keep unrelated changes out of the diff! :-)

(I don't think we want this change but we can talk about it elsewhere.)

jrose-signal · 2022-06-29T23:31:36Z

rust/protocol/src/curve.rs

+//!```
+//! use libsignal_protocol::KeyPair;
+//! use std::collections::HashSet;
+//!
+//! let alice = KeyPair::generate(&mut rand::thread_rng());
+//! assert!(alice == alice.clone());
+//! let bob = KeyPair::generate(&mut rand::thread_rng());
+//! assert!(alice != bob);
+//!
+//! // Keys can be hashed and put in sets.
+//! let key_set: HashSet<KeyPair> = [alice, bob].iter().cloned().collect();
+//! assert!(key_set.contains(&alice));
+//! assert!(key_set.contains(&bob));
+//!```


I…don't consider these very compelling examples, heh! They don't show any key cryptography things happening. I wouldn't consider that a blocker, though; the module could just have that one line of documentation for now.

jrose-signal · 2022-06-29T23:32:07Z

rust/protocol/src/curve.rs

+
+#![warn(missing_docs)]
+
+pub mod curve25519;


We talked about this a bit a while ago, but this is very deliberately not pub, because we don't want libsignal_protocol keys to be thought of as using a specific key implementation. All we need is that they support both signatures and key agreement.

jrose-signal · 2022-06-30T00:01:10Z

rust/protocol/src/curve/curve25519.rs

@@ -100,6 +131,8 @@ impl PrivateKey {
        result
    }

+    /// Verify a signature from [`Self::calculate_signature`] against another key pair's public key.


This is a static method, so there's no "another key pair" to worry about.

jrose-signal · 2022-06-30T00:01:46Z

rust/protocol/src/curve/curve25519.rs

+    /// Return the bytes of a [`PublicKey`] that another party can use to validate against
+    /// [`Self::verify_signature`].
    pub fn derive_public_key_bytes(&self) -> [u8; PUBLIC_KEY_LENGTH] {


That's not the only use, it's also for key agreement! I'd drop the use part completely again.

jrose-signal · 2022-06-30T00:02:59Z

rust/protocol/src/lib.rs

@@ -25,7 +25,7 @@
 mod address;
 mod consts;
 mod crypto;
-mod curve;
+pub mod curve;


This still counts as changing the module structure; it's the public surface of the module that matters, not its implementation. I still think we should stay flat until we discuss it properly and as a whole.

jrose-signal · 2022-06-30T00:04:49Z

rust/protocol/src/curve.rs

-        }
-    }
+/// Interface for structs that perform operations parameterized by values of [`KeyType`].
+pub trait Keyed {


Hm, this isn't really the right name for the trait. Public keys and private keys and key pairs aren't "keyed", they're…"typed"?

That said, though, I think this is a reasonable trait, but until we have a use case for it I'd rather not add it, and that won't happen until we support another key type. It might turn out we need different operations besides just key_type.

jrose-signal · 2022-06-30T00:05:43Z

rust/protocol/src/curve.rs

+    pub fn calculate_agreement(
+        &self,
+        their_key: &PublicKey,
+    ) -> Result<[u8; curve25519::AGREEMENT_LENGTH]> {


This is the place we talked about before: key agreement can't hardcode an output length, because the keys might (someday) not be curve25519 keys.

- [NON RUST/RENAME] rename "DJB_TYPE" to "CURVE_25519_TYPE" in java and swift - [1+CRYPTO CRATE] make swift/build-ffi.sh --generate-ffi work - [1+CURVE] add docs and the Keyed trait to `curve*.rs`

stale · 2023-02-04T03:56:53Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale · 2023-02-11T04:16:04Z

This issue has been closed due to inactivity.

This was referenced Jun 27, 2022

module documentation tracking issue #467

Open

Add docstrings for 7 more Rust API modules #287

Closed

cosmicexplorer force-pushed the rename-djb-curve25519 branch 4 times, most recently from 847c612 to 4b4964d Compare June 27, 2022 08:42

cosmicexplorer marked this pull request as draft June 27, 2022 08:44

cosmicexplorer marked this pull request as ready for review June 27, 2022 08:48

cosmicexplorer force-pushed the rename-djb-curve25519 branch from 4b4964d to 73ede63 Compare June 27, 2022 08:49

cosmicexplorer marked this pull request as draft June 28, 2022 05:30

jrose-signal reviewed Jun 30, 2022

View reviewed changes

cosmicexplorer marked this pull request as ready for review July 18, 2022 22:37

cosmicexplorer force-pushed the rename-djb-curve25519 branch from 73ede63 to 49c574e Compare July 19, 2022 00:02

[signalapp#475] rename everything from "djb" to just "curve25519"

6f7f325

- [NON RUST/RENAME] rename "DJB_TYPE" to "CURVE_25519_TYPE" in java and swift - [1+CRYPTO CRATE] make swift/build-ffi.sh --generate-ffi work - [1+CURVE] add docs and the Keyed trait to `curve*.rs`

cosmicexplorer force-pushed the rename-djb-curve25519 branch from 49c574e to 6f7f325 Compare September 9, 2022 20:02

stale bot added the stale label Feb 4, 2023

stale bot closed this Feb 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rename "djb" type curve to "curve25519" everywhere #475

rename "djb" type curve to "curve25519" everywhere #475

cosmicexplorer commented Jun 27, 2022 •

edited

Loading

jrose-signal left a comment

jrose-signal Jun 29, 2022

jrose-signal Jun 29, 2022

jrose-signal Jun 29, 2022

jrose-signal Jun 29, 2022

jrose-signal Jun 29, 2022

jrose-signal Jun 30, 2022

jrose-signal Jun 30, 2022

jrose-signal Jun 30, 2022

jrose-signal Jun 30, 2022

jrose-signal Jun 30, 2022

stale bot commented Feb 4, 2023

stale bot commented Feb 11, 2023

		pub const CTR_NONCE_SIZE: usize = aes::BLOCK_SIZE - 4;

		const NONCE_SIZE: usize = Self::CTR_NONCE_SIZE;

rename "djb" type curve to "curve25519" everywhere #475

rename "djb" type curve to "curve25519" everywhere #475

Conversation

cosmicexplorer commented Jun 27, 2022 • edited Loading

TODO

jrose-signal 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

stale bot commented Feb 4, 2023

stale bot commented Feb 11, 2023

cosmicexplorer commented Jun 27, 2022 •

edited

Loading