open-quantum-safe / liboqs

C library for prototyping and experimenting with quantum-resistant cryptography
https://openquantumsafe.org/
Other
1.73k stars 423 forks source link

copy_from_upstream run mistake (also in 0.9.0 release) #1586

Closed baentsch closed 9 months ago

baentsch commented 10 months ago

When looking into https://github.com/open-quantum-safe/liboqs/issues/1584 I noticed that "main" (and 0.9.0) are not idempotent to a run of copy_from_upstream on my machine: See the differences in https://github.com/open-quantum-safe/liboqs/blob/mb-fixcopyfromupstream: Compare for example the documentation of algorithm feature sets for Sphincs.

We have a test protecting against such failures, but this "silently" failed i.e., incorrectly returned OK after reporting a failure. This ought to be investigated I'd gather.

Also the Release process documentation was lacking in this regard (now fixed).

Could someone else please take a look (maybe first just confirm this finding as I'm not sure my machine is not a bit misconfigured), maybe @praveksharma or @SWilson4 ? If confirmed, we might want to add wording to the release notes of 0.9.0 that the algorithm documentation isn't "quite right": At first blush, it seems upstream Sphincs claims support for all OSs while liboqs only documents support for Linux&Darwin for optimized Sphincs... Or is this related to the limitations of our shared code in Windows? But why then doesn't copy_from_upstream reconcile this?

SWilson4 commented 10 months ago

Thanks for catching this, Michael!

Could someone else please take a look (maybe first just confirm this finding as I'm not sure my machine is not a bit misconfigured), maybe @praveksharma or @SWilson4 ? If confirmed, we might want to add wording to the release notes of 0.9.0 that the algorithm documentation isn't "quite right": At first blush, it seems upstream Sphincs claims support for all OSs while liboqs only documents support for Linux&Darwin for optimized Sphincs... Or is this related to the limitations of our shared code in Windows? But why then doesn't copy_from_upstream reconcile this?

I ran copy_from_upstream on the same commit and got slightly different results: see here https://github.com/open-quantum-safe/liboqs/commit/45f8db6cc6d0bd1432abe814a65219e07bdbf613. I've also confirmed that my branch is idempotent under copy_from_upstream. Not sure why it would be inconsistent across environments. :/

As for the Sphincs+ inconsistency with upstream, I believe the documentation is in line with what we want: we have a patch to document the OS restriction.

Edit for clarity: by "my branch" I mean the branch I created to test this issue. My local copy of main is not idempotent under copy_from_upstream.

baentsch commented 10 months ago

the documentation is in line with what we want: we have a patch to document the OS restriction.

Ah, yes, something was in the back of my mind: Thanks for the pointer. But then why didn't that get applied by copy_from_upstream??

My local copy of main is not idempotent under copy_from_upstream.

Very well, err, not well -- but at least we have a confirmed issue. Just which? #1589 does not appear to apply the sphincs patch (on my machine -- it seems to work on yours). But wait: #1589 now also has a working CI run for copy_from_upstream -- and that confirms "my" run (not properly applying the sphincs patch). What's going on here? Does copy_from_upstream silently discard failing patch applications???

bhess commented 9 months ago

I can also confirm on two instances (MacOS, Fedora Linux) that there are non-committed changes in the docs after running copy_from_upstream.py on main:

$  ! git status | grep modified
    modified:   docs/algorithms/kem/classic_mceliece.md
    modified:   docs/algorithms/kem/classic_mceliece.yml
    modified:   docs/algorithms/sig/falcon.md
    modified:   docs/algorithms/sig/sphincs.md
    modified:   docs/algorithms/sig/sphincs.yml
$ echo $?
1

After I commit the changes and run copy_from_upstream.py again, it's idempotent. I also don't see why this should be different across environments. @SWilson4 what env do you use? Might there be some python modules missing (requirements.txt) that would skip some steps?

CI seems to give a hint to a solution why it fails there before running !git status | grep modified:

fatal: detected dubious ownership in repository at '/__w/liboqs/liboqs'
To add an exception for this directory, call:

    git config --global --add safe.directory /__w/liboqs/liboqs
baentsch commented 9 months ago

That error message is fixed by my latest PR (#1589).

SWilson4 commented 9 months ago

It seems that the inconsistency between my "env" and that of @baentsch was actually due to my running copy_from_upstream with the -k switch. When running without -k, my results match Michael's. So there was no bizarre env-specific behaviour here, just the bug which will be (hopefully!) fixed in #1589.