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

RFC9381 ECVRF implementation #1188

Merged
merged 12 commits into from
May 25, 2024
Merged

Conversation

iquerejeta
Copy link
Contributor

@iquerejeta iquerejeta commented Jun 14, 2022

Implementation of RFC9381.

Made a change to ed25519_ref10 for performance reasons. Mainly, I have included a variable base variable time scalar multiplication (to compute a * U + b * V for variable U and V).

Implements #1177

test/default/vrf.c Outdated Show resolved Hide resolved
test/default/vrf Outdated Show resolved Hide resolved
test/default/vrf.c Outdated Show resolved Hide resolved
@jedisct1 jedisct1 force-pushed the master branch 4 times, most recently from 82c35a0 to 6d1d7ed Compare November 14, 2022 21:16
@jedisct1
Copy link
Owner

@iquerejeta
Copy link
Contributor Author

@jedisct1 , does this mean that we are good to push this forward? If yes, I'll make a pass to make sure that the implemented version is what got finalised in the draft, and mark it as ready.

@jedisct1
Copy link
Owner

@iquerejeta Yes :)

@iquerejeta iquerejeta marked this pull request as ready for review September 6, 2023 09:09
@iquerejeta
Copy link
Contributor Author

iquerejeta commented Sep 11, 2023

Got some trouble reproducing the undefined reference to 'crypto_core_ed25519_scalar_negate' and undefined reference to 'crypto_core_ed25519_scalar_negate' errors locally. I made sure I was configuring with the same call as the CI action:

env CPPFLAGS="-DDEV_MODE=1" ./configure --disable-dependency-tracking --enable-minimal

But couldn't reproduce it.

This is the failing action https://github.com/jedisct1/libsodium/actions/runs/6095043654

@iquerejeta
Copy link
Contributor Author

So, I've managed to reproduce them, but no progress in debugging. I'll try again tomorrow. Any idea why we might have

Undefined symbols for architecture arm64:
...
ld: symbol(s) not found for architecture arm64

type errors for symbols that are exposed in the public API when called within the library?

@jedisct1
Copy link
Owner

Errrrr... this is super weird.

How did you manage to reproduce it? Is it non-deterministic?

@jedisct1
Copy link
Owner

Actually it's not weird.

The core_ed25519 functions are not available in MINIMAL builds.

@iquerejeta
Copy link
Contributor Author

Ok, makes sense. What is your preferred way forward? To have core_ed25519 exposed in the MINIMAL build, or instead have from_string and scalar_negate as part of ed25519_ref10.c? Seeing how the library is organised, I presume the second, but just want to check with you before making further changes.

@jedisct1
Copy link
Owner

Yeah, I agree that the second option looks better.

@iquerejeta iquerejeta changed the title Version 12 of ECVRF draft RFC9381 ECVRF implementation Sep 13, 2023
@jedisct1 jedisct1 merged commit 7978205 into jedisct1:master May 25, 2024
8 checks passed
@nurupo
Copy link

nurupo commented Jul 10, 2024

Look like this is the first time a VLA was used in libsodium. At least according to ComCert, a formally verified mostly-C99 (it doesn't support VLAs) compiler intended for life-critical and mission-critical software:

libsodium/src/libsodium/crypto_vrf/rfc9381/verify.c:50: error: size of array is not a compile-time constant

Not sure if you have a no-VLA policy or anything like that, just thought I would let you know.

@jedisct1
Copy link
Owner

VLAs and alloca should not be used, as it's hard to know what the usage limits are before getting a stack overflow. And as documented, CompCert is one of the compilers libsodium is always tested with. That code hasn't been reviewed yet, but that's one of the things that will be changed before it gets merged into stable.

nurupo added a commit to nurupo/InsertProjectNameHere that referenced this pull request Jul 13, 2024
The master branch might include staging code that is subject to change,
e.g. code including VLAs, which CompCert does not support and thus would
fail on, which will be changed to not use VLAs once merged into the
stable branch.

See jedisct1/libsodium#1188 (comment)
Repository owner locked and limited conversation to collaborators Aug 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants