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

Add libsodium extension #7827

Closed
fredemmott opened this issue May 5, 2017 · 13 comments
Closed

Add libsodium extension #7827

fredemmott opened this issue May 5, 2017 · 13 comments

Comments

@fredemmott
Copy link
Contributor

Going to be part of php 7.2

Already got substantial parts of this done, just filing this issue for visibility; don't want someone to waste time reimplementing what I've already done :)

@fredemmott
Copy link
Contributor Author

For internal reference: D5015855

still need to do the AEAD functions, and there's a few non-technical blockers on getting this public, but that's the primary goal of this.

@fredemmott
Copy link
Contributor Author

All done, including AEAD, but blocked on an error in our GCC5 LTO builds only:

<strip>/libsodium/include/sodium/crypto_generichash_blake2b.h:23:0: error: type ‘struct crypto_generichash_blake2b_state’ violates one definition rule [-Werror=odr]
 typedef CRYPTO_ALIGN(64) struct crypto_generichash_blake2b_state {
 ^
<strip>/libsodium/include/sodium/crypto_generichash_blake2b.h:30:3: note: a type with different alignment is defined in another translation unit
 } crypto_generichash_blake2b_state;
   ^
lto1: all warnings being treated as errors

This doesn't make much sense to me as it's a packed struct with fixed-sized members except for size_t, and we're always using the same compiler, so CRYPTO_ALIGN isn't being redefined.

Still investigating internally - though @paragonie-scott, @jedisct1 does this look familiar at all?

@jedisct1
Copy link
Contributor

Never saw anything like that before, and indeed, it doesn't really make sense in this context :/

@fredemmott
Copy link
Contributor Author

Thanks; we're leaning towards compiler bug and have a workaround to test on Monday :)

@jedisct1
Copy link
Contributor

You rock!

@fredemmott
Copy link
Contributor Author

Another point towards compiler/linker bug: it's an extern "C" struct with no vtable, methods, etc. AIUI the linker shouldn't even know it exists.

@apinski-cavium
Copy link

apinski-cavium commented May 13, 2017

@fredemmott the linker is not what is complaining but rather the compiler during the link stage (LTO). I suspect CRYPTO_ALIGN(64) is defined differently in two Translation units. Or rather you have placed CRYPTO_ALIGN(64) in the wrong location so it applies to the typedef rather than struct name.

@denji
Copy link
Contributor

denji commented May 15, 2017

@fredemmott
Copy link
Contributor Author

fredemmott commented May 15, 2017

@apinski-cavium's suggestion is what we were going to try, and it works :) will file issue.

@fredemmott
Copy link
Contributor Author

All finished internally, diff open-sourcing it + adding cmake support is up for review :)

@jedisct1
Copy link
Contributor

Awesome!

@denji
Copy link
Contributor

denji commented May 22, 2017

@fredemmott How about supporting strong-cryptography Argon2 (PHP 7.2+)? php/php-src#1997 (https://wiki.php.net/rfc/argon2_password_hash)

@fredemmott
Copy link
Contributor Author

Please file a separate issue for argon2 support in the password_hash/verify() functions; those aren't part of libsodium, so not part of this issue.

That said, the libsodium extension does include Argon2 functionality via the sodium_crypto_pwhash* functions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants