Add crypto callback only mode for ed25519#10830
Conversation
|
Jenkins retest this please |
|
retest this please |
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10830
Scan targets checked: wolfcrypt-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
No new issues found in the changed files. ✅
|
80c6f68 to
37b0c37
Compare
|
jenkins retest this please |
rizlik
left a comment
There was a problem hiding this comment.
Great work, but the problem is that make_public and check-key survives and so ge_operations can't be removed from the build.
Given that the main goal of ONLY_ED25519 is to reduce code/data size, we should aim at removing all the software math.
I see two options:
- Gating out the make_public and check_key with new gating macros
- Adding two crypto callbacks for make_public and check_key
Frauschi
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: COMMENT
Findings: 1 total — 1 posted, 0 skipped
Posted findings
- [Low] Sign CB_ONLY path omits (void)ed25519Ctx suppression present in verify path —
wolfcrypt/src/ed25519.c:445
Review generated by Skoll via Claude/Codex
67cd0ad to
d4cb29c
Compare
Option 2 added, following the pattern from ECC. The ge operations are completely gone, but I did have to inline a small chunk of math. |
8379f8d to
ef4e304
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10830
Scan targets checked: wolfcrypt-bugs, wolfcrypt-port-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Findings: 3
3 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
031d58b to
9a941fb
Compare
rizlik
left a comment
There was a problem hiding this comment.
WOLFSSL_CURVE25519_USE_ED25519 auto-define in fe_operations.h re-enable the math.
The config --enable-ed25519 --enable-cryptocb --disable-curve25519 + ONLY_ED25519 fails the linkage.
Probably disabling blinding also re-enable the math silently.
My suggestion is to gate the USE_ED25519 to !CB_ONLY_ED25519
If you prefer to do that in a follow-up PR I'm OK also to merge as it is now |
Excellent catch. I've pushed a commit to fix it and add a test case in CI. For future reference, this was the problematic configure command: |
614585f to
a6383a3
Compare
philljj
left a comment
There was a problem hiding this comment.
Looks good first pass, just two tiny questions.
@philljj it can be addressed by the PR that adds |
|
Retest this please. (Connection to github.com closed by remote host.). |
Description
Expand existing ed25519 crypto callback to support the "only" mode which strips the software implementation for use with HSM or other hardware accelerators.
make_key,check_key,make_pub,sign_msgandverify_msgsupported. No streaming support.Testing
Added unit test and updated CI workflow
Checklist