Review of libpep by Hans

Het ziet er goed uit. Ik heb wel wat vragen/opmerkingen:

  1. Ik heb const char* moeten veranderen in auto in base.cpp regel 233 om te kunnen compileren met MSVC

  2. Waarom andere C++ standaard voor Android? Betekent dat niet dat je dan sowieso alleen maar C++17 features kunt gebruiken? Of, als je #ifdef gebruikt, dat je in ieder geval een C++17 implementatie moet leveren. Kun je dan niet net zo goed alleen C++17 gebruiken?

    Het compileert voor mij sowieso niet met C++17:

    src\core.cpp(16): error C2039: 'invalid_argument': is not a member of 'std'
include\base.h(117): error C2668: 'radboud::pep::_SHA512Update': ambiguous call to overloaded function

    Die eerste is makkelijk op te lossen door #include <stdexcept> toe te voegen.

  3. In lib-common.h: werkt clang diagnostic push ook in gcc? Dit wordt gebruikt in een blok #if defined(__clang__) || defined(__GNUC__)

  4. Ik zou de voorkeur geven aan Scalar::is_zero, ipv Scalar::zero. Duidelijker dat het een boolean is, en niet je scalar op 0 zet oid. Scalar::valid vind ik minder ambigu, maar zou ik dan ook is_valid van maken. Idem voor GroupElement.

  5. Ik vind Scalar::base een onduidelijke naam voor wat het doet. (kan aan mij liggen, misschien is het gebruikelijke terminologie) mult_base oid zou ik duidelijker vinden

  6. Scalar::invert controleert niet of de input 0 is, of de return value van crypto_core_ristretto255_scalar_invert die -1 returnt als de input 0 is. Moet dat niet gecontroleerd worden?

  7. Goed dat de error in Scalar::base een hint geeft over wat er mis kan zijn. Op basis van de libsodium-documentatie zou ik zelfs zeggen dat je 'probably' weg kunt laten.

  8. In GroupElement::operator+ en GroupElement::operator- wordt de return-value van de gebruikte libsodium-functie niet gecheckt. Libsodium zegt bij beide functies:

    The function returns 0 on success, or -1 if p and/or q are not valid encoded elements.

    Ga je er vanuit dat ze nooit invalid kunnen zijn (door de check in FromHex)? Dit zou je expliciet kunnen maken met EXPECT.

  9. Scalar::operator== en GroupElement::operator== zijn niet constant-time. Alle gebruikte libsodium-functies zijn dat dacht ik wel. Is dat iets wat we hier ook willen?

  10. In RandomBytes gebruik je een functie die niet in de libsodium documentatie genoemd wordt. Hij lijkt er vooral vanwege NaCl-compatibiliteit in te zitten. Waarom gebruik je niet randombytes_buf?

  11. RerandomizeY voert een voor mij onbekende actie uit. De functie wordt ook nergens gebruikt, en staat ook niet in core.h. Dus wat is hier precies de bedoeling van?