Skip to content

x448: initial import #1250

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Aug 20, 2025
Merged

x448: initial import #1250

merged 3 commits into from
Aug 20, 2025

Conversation

baloo
Copy link
Member

@baloo baloo commented Jun 9, 2025

@baloo
Copy link
Member Author

baloo commented Jun 9, 2025

cc @dignifiedquire @kevaundray

@baloo baloo mentioned this pull request Jun 9, 2025
Closed
@baloo baloo force-pushed the baloo/x448-initial-import branch 6 times, most recently from 48849e0 to c67e30d Compare June 11, 2025 05:59
@baloo baloo force-pushed the baloo/x448-initial-import branch from c67e30d to 8675b5f Compare June 17, 2025 16:12
tarcieri
tarcieri reviewed Jun 19, 2025
View reviewed changes
@baloo
Copy link
Member Author

baloo commented Jul 10, 2025

This kind of broke when we merged #1284

cc @daxpedda

I'm confused. I don't understand how https://www.rfc-editor.org/rfc/rfc7748#section-5 asks for the scalar to be a 56-bytes string and now the EdwardsScalar is 57 bytes.

Am I supposed to use the DecafScalar instead?

@daxpedda
Copy link
Contributor

Apologies, I noticed indeed that this should have been DecafScalar.
I'm currently preparing another PR where I try to fix those names, because having a separate scalar for each curve when only one is special, Ed448, is a bit much.

Feel free to fix this in this PR.
Basically just replace all uses of EdwardsScalar in montgomery.rs with DecafScalar.

@baloo
Copy link
Member Author

baloo commented Jul 10, 2025

No worries, I'm not sure I'm following what you're saying, I guess I'll wait for your PR ^^

@baloo baloo force-pushed the baloo/x448-initial-import branch 3 times, most recently from 89ee8ef to 2280752 Compare July 16, 2025 19:01
@baloo baloo marked this pull request as draft July 16, 2025 19:02
@baloo baloo force-pushed the baloo/x448-initial-import branch from 2280752 to 7368a67 Compare July 18, 2025 20:18
This was referenced Jul 18, 2025
Merged
Closed
@baloo baloo force-pushed the baloo/x448-initial-import branch 3 times, most recently from 875180d to f8cd31a Compare July 18, 2025 20:26
@daxpedda
Copy link
Contributor

I have a new and improved PR up for MontgomeryScalar: #1306.

@baloo baloo force-pushed the baloo/x448-initial-import branch 6 times, most recently from 42c40da to 9138194 Compare July 19, 2025 23:43
@tarcieri
Copy link
Member

@baloo is there a specific reason this is still draft? It'd be good to get it landed so we make sure it gets updated when ed448-goldilocks is changed

@daxpedda
Copy link
Contributor

Its currently relying on #1306.
I could try to extract MontgomeryScalar from #1306 to make it easily reviewable first if that's desirable.

@baloo baloo force-pushed the baloo/x448-initial-import branch 3 times, most recently from 36aa271 to 87d027d Compare July 20, 2025 15:28
@baloo
Copy link
Member Author

baloo commented Jul 20, 2025

@baloo is there a specific reason this is still draft? It'd be good to get it landed so we make sure it gets updated when ed448-goldilocks is changed

I struggled for a bit with various Scalars, #1306 fixed it for me.

I'll keep rebasing and testing along with the merges until we can get this out of draft :)

@baloo baloo force-pushed the baloo/x448-initial-import branch 2 times, most recently from a9bb6e9 to 0c69dd6 Compare July 20, 2025 18:16
@baloo baloo marked this pull request as ready for review July 20, 2025 18:16
@baloo baloo mentioned this pull request Jul 20, 2025
Draft
@baloo
Copy link
Member Author

baloo commented Jul 20, 2025

I guess the scalar wasn't that hard to figure out in the end.

I've sent a PR over to @daxpedda 's branch to migrate over to MontgomeryXpoint khonsulabs#1

@baloo baloo force-pushed the baloo/x448-initial-import branch from 0c69dd6 to 7f10832 Compare July 23, 2025 16:01
@tarcieri tarcieri mentioned this pull request Jul 25, 2025
Open
11 tasks
@baloo baloo force-pushed the baloo/x448-initial-import branch from 7f10832 to 254c30a Compare August 5, 2025 22:38
@baloo
Copy link
Member Author

baloo commented Aug 6, 2025

Is there anything else required here?

baloo and others added 3 commits August 19, 2025 20:10
@baloo baloo force-pushed the baloo/x448-initial-import branch from 254c30a to 15dd588 Compare August 20, 2025 03:11
tarcieri
tarcieri approved these changes Aug 20, 2025
View reviewed changes
Copy link
Member

@tarcieri tarcieri left a comment

Choose a reason for hiding this comment

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

Since this has been dragging on for awhile, I'm going to go ahead and merge it, but I have a few notes on the API I'll leave afterward and maybe we can make some followup changes.

@tarcieri tarcieri merged commit 6b8726d into RustCrypto:master Aug 20, 2025
174 checks passed
tarcieri
tarcieri reviewed Aug 20, 2025
View reviewed changes
x448/src/lib.rs
Comment on lines +115 to +128
/// Performs a Diffie-hellman key exchange between the secret key and an external public key
pub fn as_diffie_hellman(&self, public_key: &PublicKey) -> Option<SharedSecret> {
// Check if the point is one of the low order points
if public_key.0.is_low_order() {
return None;
}
let shared_key = &public_key.0 * &self.as_scalar();
Some(SharedSecret(shared_key))
}

/// Performs a Diffie-hellman key exchange once between the secret key and an external public key
pub fn to_diffie_hellman(self, public_key: &PublicKey) -> Option<SharedSecret> {
self.as_diffie_hellman(public_key)
}
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem like a helpful distinction. I think we should just have the borrowing version. Even for ephemeral D-H keys, there are valid reasons to perform more than one D-H operation (i.e. 3DH)

tarcieri
tarcieri reviewed Aug 20, 2025
View reviewed changes
x448/src/lib.rs
Comment on lines +132 to +138
// First check if we have 56 bytes
if bytes.len() != 56 {
return None;
}

let secret = Secret::from(slice_to_array(bytes));
Some(secret)
Copy link
Member

Choose a reason for hiding this comment

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

Seems like this could use TryFrom

tarcieri
tarcieri reviewed Aug 20, 2025
View reviewed changes
x448/src/lib.rs
Comment on lines +141 to +144
/// Converts a secret into a byte array
pub fn as_bytes(&self) -> &[u8; 56] {
&self.0
}
Copy link
Member

Choose a reason for hiding this comment

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

We should probably have different types for ephemeral vs static secrets ala x25519-dalek. For ephemeral secrets ideally the caller couldn't export this value (for forward secrecy).

@baloo baloo deleted the baloo/x448-initial-import branch August 20, 2025 19:03
@baloo baloo mentioned this pull request Aug 21, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tarcieri tarcieri tarcieri approved these changes

+2 more reviewers

@anonion0 anonion0 anonion0 left review comments

@kevaundray kevaundray kevaundray left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@baloo @daxpedda @tarcieri @anonion0 @kevaundray