Add sig api with picnic #120

christianpaquin · 2017-05-11T19:20:42Z

This pull request proposes a new signature API modeled after the kex api, and adds the Picnic signature scheme.

Picnic must currently be downloaded and set up (to generate parameters) using a script (download-and-setup-picnic.sh). We can migrate to a git submodule at a later time, after this PR is reviewed.

The Visual Studio build has not yet been modified to support Picnic; this will be done after this PR is reviewed.

…ipt executable, added TODO in test_sig.

…. hardcoded).

smashra · 2017-06-26T01:50:26Z

I'm getting following error, please fix:
external/Picnic-master/LowMC.c: In function 'getAllRandomness':
external/Picnic-master/LowMC.c:209:20: error: storage size of 'ctx' isn't known
EVP_CIPHER_CTX ctx;
^~~
external/Picnic-master/LowMC.c:209:20: error: unused variable 'ctx' [-Werror=unused-variable]
external/Picnic-master/LowMC.c: In function 'cleanup_EVP':
external/Picnic-master/LowMC.c:245:5: error: 'ERR_remove_state' is deprecated [-Werror=deprecated-declarations]
ERR_remove_state(0);
^~~~~~~~~~~~~~~~
In file included from /usr/local/include/openssl/bn.h:31:0,
from /usr/local/include/openssl/asn1.h:24,
from /usr/local/include/openssl/objects.h:15,
from /usr/local/include/openssl/evp.h:28,
from external/Picnic-master/LowMC.c:32:
/usr/local/include/openssl/err.h:249:1: note: declared here
DEPRECATEDIN_1_0_0(void ERR_remove_state(unsigned long pid))
^
cc1: all warnings being treated as errors

tlepoint · 2017-06-28T19:05:19Z

Apparently, picnic is in master? (but without test_sig?)

dstebila · 2017-06-29T08:47:12Z

Yeah there's been a bit of confusion about this PR and what should be added. I'm working with @smashra to try to sort it out and will update you on here in the next few days.

smashra · 2017-06-29T13:20:25Z

Removing make and build steps from download-and-setup-picnic.sh as they are being taken care of from the main Makefile with configure options in configure.ac.

smashra · 2017-06-29T14:14:47Z

Made
#include <graycode.h>
to
#include <m4ri/graycode.h>
in preprocessmatrices.c

for making --with-m4ri-dir option consistent with others.

Please update your branch.

smashra · 2017-06-29T14:40:30Z

Can you please create another pull request to fix this:

Made
#include <graycode.h>
to
#include <m4ri/graycode.h>
in preprocessmatrices.c

for making --with-m4ri-dir option consistent with others.

Such that I can check in picnic code.

smashra · 2017-06-29T14:41:24Z

Sorry, the bold and caps was a mistake, totally unintentional.

smashra · 2017-06-30T14:53:38Z

Please fix these warning:

src/sig_picnic/external/Picnic-master/preprocessMatrices.c:70:23: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
for (int i = 0; i < numRounds; i++) {
^
src/sig_picnic/external/Picnic-master/preprocessMatrices.c:72:27: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
for (int j = 0; j < stateBits / t; j++) {
^
src/sig_picnic/external/Picnic-master/preprocessMatrices.c:80:23: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
for (int i = 0; i < numRounds; i++) {
^
src/sig_picnic/external/Picnic-master/preprocessMatrices.c:82:27: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
for (int j = 0; j < stateBits / t; j++) {
^
src/sig_picnic/external/Picnic-master/preprocessMatrices.c:93:23: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
for (int i = 0; i < numRounds; i++) {
^
src/sig_picnic/external/Picnic-master/preprocessMatrices.c:94:27: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
for (int j = 0; j < stateBits / t; j++) {
^
src/sig_picnic/external/Picnic-master/preprocessMatrices.c:105:19: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
for (i = 0; i < numRounds; i++) {
^
src/sig_picnic/external/Picnic-master/preprocessMatrices.c:107:23: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
for (j = 0; j < stateBits / t; j++) {
^
src/sig_picnic/external/Picnic-master/preprocessMatrices.c:111:31: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
for (l = 0; l < stateWords; l++) {
^
src/sig_picnic/external/Picnic-master/preprocessMatrices.c:113:27: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
if (l != stateWords - 1) {
^~
src/sig_picnic/external/Picnic-master/preprocessMatrices.c:126:19: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
if (j != (stateBits / t) - 1) {
^~
src/sig_picnic/external/Picnic-master/preprocessMatrices.c:132:15: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
if (i != numRounds - 1) {
^~
src/sig_picnic/external/Picnic-master/preprocessMatrices.c:138:23: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
for (int i = 0; i < numRounds; i++) {
^
src/sig_picnic/external/Picnic-master/preprocessMatrices.c:139:27: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
for (int j = 0; j < stateBits / t; j++) {
^
src/sig_picnic/external/Picnic-master/preprocessMatrices.c: In function ‘writeRoundConstants’:
src/sig_picnic/external/Picnic-master/preprocessMatrices.c:152:23: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
for (int i = 0; i < numRounds; i++) {
^
src/sig_picnic/external/Picnic-master/preprocessMatrices.c: In function ‘generatelookupTables’:
src/sig_picnic/external/Picnic-master/preprocessMatrices.c:180:5: error: enumeration value ‘PARAMETER_SET_INVALID’ not handled in switch [-Werror=switch]
switch (picnic_params) {
^~~~~~
src/sig_picnic/external/Picnic-master/preprocessMatrices.c:180:5: error: enumeration value ‘PARAMETER_SET_MAX_INDEX’ not handled in switch [-Werror=switch]

dstebila · 2017-06-30T14:56:22Z

Since Picnic-master is an external dependency, can we compile that directory without -Werror while the upstream provider considers whether to adopt the changes or not?

smashra · 2017-06-30T14:58:40Z

Ok.

…

On Fri, Jun 30, 2017 at 10:56 AM, Douglas Stebila ***@***.***> wrote: Since Picnic-master is an external dependency, can we compile that directory without -Werror while the upstream provider considers whether to adopt the changes or not? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#120 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ANmJw2w_lpbi29-W1cF6qF6gUrqc42_Zks5sJQyWgaJpZM4NYcKN> .

smashra · 2017-07-04T14:09:11Z

I'm already getting name collision error:

./.libs/liboqs.a(libpicnic_la-LowMC.o): In function verify': /home/shravan/workspace/delete_picnic/liboqs/src/sig_picnic/external/Picnic-master/LowMC.c:1050: multiple definition of verify'
./.libs/liboqs.a(libkyber_la-kex_mlwe_kyber.o):/home/shravan/workspace/delete_picnic/liboqs/src/kex_mlwe_kyber/verify.c:10: first defined here
collect2: error: ld returned 1 exit status
make[2]: *** [Makefile:732: test_sig] Error 1

Douglas, we discussed to not enable picnic by default but why does
src/kex_mlwe_kyber/verify.c
has a global function verify() and still it has been accepted in master and enabled by default.

dstebila · 2017-07-04T14:30:06Z

@smashra Apparently there were non-namespaced names added in the merge of Kyber (#131) and these weren't caught by Travis since Travis isn't correctly catching all failures (open issue #130). So I think the sequence of operations would be:

Prepare a pull request that fixes Non-namespaced global symbols: Travis should fail even more... #130, which will mean current builds fail.
Prepare a pull request that builds on the pull request in 1. and that fixes the non-namespaced symbols in Kyber.
Merge both of those to master.
Update this pull request.

christianpaquin · 2017-07-18T19:14:14Z

@smashra, the last commit #6153022 removed the Picnic matrices preprocessing in the download-and-setup-picnic.sh script. How do you propose we setup Picnic after downloading it?

smashra · 2017-07-18T19:18:10Z

It's done through root or main make file.am. You'll see when you run make test.

…

On Tue, Jul 18, 2017 at 3:14 PM Christian Paquin ***@***.***> wrote: @smashra <https://github.com/smashra>, the last commit #6153022 removed the Picnic matrices preprocessing in the download-and-setup-picnic.sh script. How do you propose we setup Picnic after downloading it? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#120 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ANmJw_PNrdA2s50DX2BFvJj1rwqLFV3Fks5sPQQHgaJpZM4NYcKN> .

christianpaquin · 2017-07-18T20:58:30Z

OK, works for me. Is there a reason why the matrices preprocessing is not part of the make (vs. make test)? I updated the README to include "make test" as a step when building Picnic. When are you planning to merge this with master? I'm working on fixing the Windows build; if this is going in soon, I'll wait to also include the sig api and Picnic.

smashra · 2017-07-18T21:13:28Z

The reason is consistency on my end with respect to passing compiler and directory flags, considering that it is an external component, such that it appears more like an internal project from build perspective. Please wait for an hour, I'm in the process of merging some mac related updates. Thanks for waiting.

…

On Tue, Jul 18, 2017 at 4:58 PM, Christian Paquin ***@***.***> wrote: OK, works for me. Is there a reason why the matrices preprocessing is not part of the make (vs. make test)? I updated the README to include "make test" as a step when building Picnic. When are you planning to merge this with master? I'm working on fixing the Windows build; if this is going in soon, I'll wait to also include the sig api and Picnic. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#120 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ANmJw-uxu8ewizhVPdkwraHBOfe8e__3ks5sPRx2gaJpZM4NYcKN> .

…npaquin/liboqs into add-sig-api-with-picnic

smashra · 2017-07-18T21:25:54Z

@christian: I have merged my changes, please go ahead. On Tue, Jul 18, 2017 at 5:13 PM, Shravan Mishra <shravan.mishra@gmail.com> wrote:

…

The reason is consistency on my end with respect to passing compiler and directory flags, considering that it is an external component, such that it appears more like an internal project from build perspective. Please wait for an hour, I'm in the process of merging some mac related updates. Thanks for waiting. On Tue, Jul 18, 2017 at 4:58 PM, Christian Paquin < ***@***.***> wrote: > OK, works for me. Is there a reason why the matrices preprocessing is not > part of the make (vs. make test)? I updated the README to include "make > test" as a step when building Picnic. When are you planning to merge this > with master? I'm working on fixing the Windows build; if this is going in > soon, I'll wait to also include the sig api and Picnic. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#120 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/ANmJw-uxu8ewizhVPdkwraHBOfe8e__3ks5sPRx2gaJpZM4NYcKN> > . >

christianpaquin · 2017-07-18T23:22:22Z

I tested the build with your commits @smashra; looks good.

smashra · 2017-07-18T23:25:53Z

Great!

…

On Tue, Jul 18, 2017 at 7:22 PM Christian Paquin ***@***.***> wrote: I tested the build with your commits @smashra <https://github.com/smashra>; looks good. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#120 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ANmJw1LU_PWyDm2Dh0KiX3BcRPlMEtrjks5sPT4vgaJpZM4NYcKN> .

tlepoint · 2017-07-18T23:46:28Z

I get a warning during compilation on macOS

  CC       src/sig_picnic/external/Picnic-master/pp_matrices-preprocessMatrices.o
src/sig_picnic/external/Picnic-master/preprocessMatrices.c:180:13: warning: enumeration values
      'PARAMETER_SET_INVALID' and 'PARAMETER_SET_MAX_INDEX' not handled in switch [-Wswitch]
    switch (picnic_params) {
            ^
1 warning generated.

…api-with-picnic # Conflicts: # .travis-tests.sh # Makefile.am

dstebila · 2017-07-25T12:17:24Z

@smashra There's still a failure in one of the macOS builds due to a header not being in the right location. Can you look into that?

smashra · 2017-07-25T12:29:53Z

Is it beacause of the merge? I'll look into it.

…

On Tue, Jul 25, 2017 at 8:17 AM Douglas Stebila ***@***.***> wrote: @smashra <https://github.com/smashra> There's still a failure in one of the macOS builds due to a header not being in the right location. Can you look into that? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#120 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ANmJw7ViObm-uQCO7crF783emwxRcONpks5sRdzVgaJpZM4NYcKN> .

smashra · 2017-07-25T13:01:02Z

I'm looking at https://travis-ci.org/open-quantum-safe/liboqs/builds/254945234?utm_source=github_status&utm_medium=notification where one of the mac has sig_picnic enabled. I don't see any errors. Where should I look ? Sincerely Shravan On Tue, Jul 25, 2017 at 8:29 AM, Shravan Mishra <shravan.mishra@gmail.com> wrote:

…

Is it beacause of the merge? I'll look into it. On Tue, Jul 25, 2017 at 8:17 AM Douglas Stebila ***@***.***> wrote: > @smashra <https://github.com/smashra> There's still a failure in one of > the macOS builds due to a header not being in the right location. Can you > look into that? > > — > You are receiving this because you were mentioned. > > > Reply to this email directly, view it on GitHub > <#120 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/ANmJw7ViObm-uQCO7crF783emwxRcONpks5sRdzVgaJpZM4NYcKN> > . >

…api-with-picnic

dstebila

There are a bunch of changes that seem unrelated to Picnic, and I think might be artifacts of merges with master. Shravan, can you please check to see what the code actually should be, as in some cases it doesn't reflect what's in master now?

dstebila · 2017-07-27T01:21:32Z

configure.ac

 ARG_ENABL_SET([kex-sidh-iqc-ref], [enable KEX-SIDH-IQC-REF.])
 AM_CONDITIONAL([kex_sidh_iqc_ref], [test "x$kex_sidh_iqc_ref" = xtrue])
 AM_CONDITIONAL([USE_SIDH_IQC], [test "x$kex_sidh_iqc_ref" = xtrue])

+
+AM_CPPFLAGS="-g -std=gnu11 -Wno-unused-function -Werror -Wpedantic -Wall -Wextra -DCONSTANT_TIME"


Why is this line changing?

It's a duplicate, there is another AM_CPPFLAGS below. Needs to be removed. Merge artifact.

dstebila · 2017-07-27T01:21:54Z

configure.ac

@@ -120,6 +126,35 @@ case $host_os in
        ;;
 esac

+AC_ARG_WITH(


Why are lines 129-149 changing? Doesn't seem related to the Picnic commit.

dstebila · 2017-07-27T01:22:22Z

configure.ac

@@ -129,6 +164,8 @@ SRCDIR=${SRCDIR}" src/crypto/aes src/crypto/rand src/crypto/sha3 src/crypto/rand

 # KEX
 SRCDIR=${SRCDIR}" src/kex"
+SRCDIR=" src/common src/crypto/aes src/kex src/sig src/crypto/rand src/crypto/sha3"
+SRCDIR=${SRCDIR}" src/crypto/rand_urandom_aesctr src/crypto/rand_urandom_chacha20"


Why is this line changing? Doesn't seem related to the Picnic commit.

Again this whole block is duplicated. Merge didn't go well.

dstebila · 2017-07-27T01:22:32Z

configure.ac

+  AM_CPPFLAGS=${AM_CPPFLAGS}" -DENABLE_NTRU"
+  SRCDIR=${SRCDIR}" src/kex_ntru"
+fi
+


Why are these lines changing? Doesn't seem related to the Picnic commit.

if test x"$kex_ntru" = x"true"; then
AM_CPPFLAGS=${AM_CPPFLAGS}" -DENABLE_NTRU"
SRCDIR=${SRCDIR}" src/kex_ntru"
fi

should not be there because its enabled by default.

dstebila · 2017-07-27T01:22:45Z

configure.ac

@@ -158,6 +200,12 @@ if test x"$kex_code_mcbits" = x"true"; then
 fi
 if test x"$kex_sidh_iqc_ref" = x"true"; then
  AM_CPPFLAGS=${AM_CPPFLAGS}" -DENABLE_SIDH_IQC_REF"
+  SRCDIR=${SRCDIR}" src/kex_sidh_iqc_ref"


Why is this line changing? Doesn't seem related to the Picnic commit.

Its more like bringing the am_cppflags and srcdir under one if conditional.

dstebila · 2017-07-27T01:23:20Z

set-picnic-var.sh

+#!/bin/sh
+export PICNIC_PARAMS_PATH=$PWD/src/sig_picnic/external/Picnic-master/precomputed_data/
+echo $PICNIC_PARAMS_PATH
+


What's the purpose of this script? There's no explanation in the README of its role.

I'm not using it anywhere. Because precomputed data is created by Makefile.am there is no additional setup required.

Okay, so can we delete it?

Yes, we can delete it.

dstebila · 2017-07-27T01:23:30Z

src/crypto/rand/rand.h

@@ -20,7 +20,7 @@ typedef struct OQS_RAND OQS_RAND;
 /**
 * OQS PRNG object
 */
-typedef struct OQS_RAND {
+struct OQS_RAND {


Why is this changing?

clang was giving error on doing
typedef struct OQS_RAND {

}OQS_RAND;

This should be

typedef struct OQS_RAND OQS_RAND; struct OQS_RAND { ... };

The above change already exists in rand.h.

Ah, I see, thanks.

* master: Enable or disable each algorithm (open-quantum-safe#158) Add sig api with picnic (open-quantum-safe#120) AppVeyor badge should point to master branch. copy header files instead of link (open-quantum-safe#157) Windows continuous integration (open-quantum-safe#155) Bring macOS build config closer to original. Try a few changes to see if we can narrow down the bug. Switch to a different version of Xcode and set travis-tests to fail on error. Fix windows build july2017 (open-quantum-safe#151) Fix unknown pseudo-op: .global under macOS (open-quantum-safe#152)

christianpaquin added 6 commits May 11, 2017 15:10

Added sig API and Picnic signature algs.

fea8c7e

Removed ifdef around Picnic defines, to avoid modifying calling apps.

d8fdde4

Some clean-ups: updated Picnic reference in Readme, made download scr…

b088e1e

…ipt executable, added TODO in test_sig.

Removed const-removing cast since Picnic API now uses const.

2c8f0e9

Revamped test_sig, cleaned-up sig and sig_picnic.

098ef9f

Enables setting location of picnic params through an env variable (vs…

5fc41f7

…. hardcoded).

Minor change.

1517051

dstebila mentioned this pull request Jul 4, 2017

Kyber has non-namespaced global symbols #137

Closed

merge picnic pull request

f732456

dstebila mentioned this pull request Jul 4, 2017

Christianpaquin add sig api with picnic #138

Closed

smashra added 8 commits July 5, 2017 20:50

Enable travis test for picnic

369dcca

Fix yml error

8a40f1e

Fix yml error

0726cd2

Fix apt package error

4da48ac

Fix include for picnic

11506d7

Exit on first error

2492421

Setup picnic external

44d9a46

Fix a typo

6ef4034

smashra and others added 3 commits July 18, 2017 10:43

Another typo

1199c3c

add picnic build for mac

016da08

formatting

97820ae

Updated README after Picnic refactoring.

e61b5fc

Merge branch 'add-sig-api-with-picnic' of https://github.com/christia…

6a9b376

…npaquin/liboqs into add-sig-api-with-picnic

Tancrede Lepoint and others added 4 commits July 18, 2017 16:57

Update and uniformization of README.md

5747474

Minor typos in README.md

509673b

Merge remote-tracking branch 'open-quantum-safe/master' into add-sig-…

d6260c7

…api-with-picnic # Conflicts: # .travis-tests.sh # Makefile.am

Fix merge mistake.

2b8e899

Shravan Mishra and others added 3 commits July 25, 2017 12:13

Fix M4RI_DIR path

6682e49

Merge remote-tracking branch 'open-quantum-safe/master' into add-sig-…

d46e14b

…api-with-picnic

Undo README changes.

9d3e30c

dstebila requested changes Jul 27, 2017

View reviewed changes

smashra and others added 2 commits July 27, 2017 10:10

Removing merge artifacts

b6361c6

File not being used anywhere

094e553

dstebila approved these changes Jul 31, 2017

View reviewed changes

dstebila merged commit 4987563 into open-quantum-safe:master Aug 1, 2017

christianpaquin deleted the add-sig-api-with-picnic branch October 10, 2018 17:55

This pull request was closed.

Add sig api with picnic #120

Add sig api with picnic #120

Conversation

christianpaquin commented May 11, 2017 • edited Loading

smashra commented Jun 26, 2017

tlepoint commented Jun 28, 2017 • edited Loading

dstebila commented Jun 29, 2017

smashra commented Jun 29, 2017

smashra commented Jun 29, 2017

smashra commented Jun 29, 2017

Can you please create another pull request to fix this:

for making --with-m4ri-dir option consistent with others.

smashra commented Jun 29, 2017

smashra commented Jun 30, 2017

dstebila commented Jun 30, 2017

smashra commented Jun 30, 2017 via email

smashra commented Jul 4, 2017

dstebila commented Jul 4, 2017

christianpaquin commented Jul 18, 2017

smashra commented Jul 18, 2017 via email

christianpaquin commented Jul 18, 2017

smashra commented Jul 18, 2017 via email

smashra commented Jul 18, 2017 via email

christianpaquin commented Jul 18, 2017

smashra commented Jul 18, 2017 via email

tlepoint commented Jul 18, 2017

dstebila commented Jul 25, 2017

smashra commented Jul 25, 2017 via email

smashra commented Jul 25, 2017 via email

dstebila 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

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

christianpaquin commented May 11, 2017 •

edited

Loading

tlepoint commented Jun 28, 2017 •

edited

Loading