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

sm2: add SM2PKE support #1069

Merged
merged 5 commits into from
Sep 5, 2024
Merged

Conversation

heliannuuthus
Copy link
Contributor

@heliannuuthus heliannuuthus commented Aug 12, 2024

  1. encrypt
  2. decrypt
  3. encrypt_der
  4. decrypt_der

@heliannuuthus heliannuuthus force-pushed the feat-sm2pke branch 2 times, most recently from a6274c3 to 0f943a3 Compare August 13, 2024 03:01
@heliannuuthus
Copy link
Contributor Author

#1067

@tarcieri
Copy link
Member

Looks great on a first pass. I'll try to do a more in-depth pass soon.

sm2/src/pke.rs Outdated Show resolved Hide resolved
sm2/src/pke.rs Outdated Show resolved Hide resolved
sm2/src/pke/encrypting.rs Outdated Show resolved Hide resolved
sm2/src/pke/encrypting.rs Outdated Show resolved Hide resolved
sm2/Cargo.toml Outdated Show resolved Hide resolved
sm2/Cargo.toml Outdated Show resolved Hide resolved
Comment on lines +167 to +168
// // If 𝑡 is an all-zero bit string, go to A1.
// if all of t are 0, xor(c2) == c2
Copy link
Member

Choose a reason for hiding this comment

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

This comment seems a little odd

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to explain why we need to determine that xor(c2) == c2,but the description seems odd

Copy link
Member

Choose a reason for hiding this comment

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

At the very least, there's a double // //

It seems like If 𝑡 is an all-zero bit string and if all of t are 0 describe the same condition in slightly different ways.

let mut ct: i32 = 0x00000001;
let mut offset = 0;
let digest_size = hasher.output_size();
let mut ha = vec![0u8; digest_size];
Copy link
Member

Choose a reason for hiding this comment

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

This won't work on heapless no_std targets, nor is the alloc feature being enabled by the pke feature.

It seems like you could get rid of these dynamic allocations if you switched from DynDigest to a generic D parameter like you have for decrypt_digest and bound on the OutputSizeUser trait, which will give you access to a compile-time output size which can be used to allocate an appropriately sized array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I once saw an issue related to using OutputSizeUser::USIZE to create a new array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I once saw an issue related to using OutputSizeUser::USIZE to create a new array.

I tried to solve this problem with the hybird_array, but I needed a little time to adjust the other places where the alloc feature was used

kdf(hasher, c1_point, &mut c2)?;

// compute 𝑢 = 𝐻𝑎𝑠ℎ(𝑥2 ∥ 𝑀′∥ 𝑦2).
let mut u = vec![0u8; digest_size];
Copy link
Member

Choose a reason for hiding this comment

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

Similar comment to this one regarding the dynamic allocations. They're unnecessary.

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.

Will go ahead and approve this and fix up my nits retroactively

@tarcieri tarcieri changed the title feat(sm2): sm2pke sm2: add SM2PKE support Sep 5, 2024
@tarcieri tarcieri merged commit 4781762 into RustCrypto:master Sep 5, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants