Add ext/sodium for PHP 7.2.0 #2560

paragonie-scott · 2017-06-06T14:49:12Z

remicollet · 2017-06-06T14:57:19Z

ext/sodium/config.m4

+
+  PHP_SUBST(LIBSODIUM_SHARED_LIBADD)
+
+  PHP_NEW_EXTENSION(libsodium, libsodium.c, $ext_shared)


Already discussed, please, drop lib prefix, so only "sodium"

Good point. @sgolemon suggested starting a PR just to get it in the door before June 15, then we'd make any changes afterwards, so I was just dumping it as-is. :P

remicollet · 2017-06-06T14:59:14Z

ext/sodium/config.m4

+        AC_MSG_RESULT(found in $i)
+      fi
+    done
+  fi


Using pkg-config output would be better than searching in some hardcoded path

I'm not sure how to do that.

See some other ext for example (zip, interbase, ...)
Else I will take care of it.

remicollet · 2017-06-06T15:10:21Z

Please drop EXPERIMENTAL as obviously no more experimental....

remicollet · 2017-06-06T15:11:42Z

ext/sodium/php_libsodium.h

+extern zend_module_entry libsodium_module_entry;
+#define phpext_libsodium_ptr &libsodium_module_entry
+
+#define PHP_LIBSODIUM_VERSION "2.0.0"


Do you want to keep it sync with pecl for older version ?
Else, could make sense to drop specific version (and use PHP version as for most of other extensions)

remicollet · 2017-06-06T15:24:18Z

ext/sodium/config.m4

@@ -1,5 +1,5 @@
 dnl $Id$
-dnl config.m4 for extension libsodium
+dnl config.m4 for extension sodium

 PHP_ARG_WITH(libsodium, for libsodium support,
 [  --with-libsodium[[=DIR]]  Include libsodium support])


I think we need 2 options

one for the ext (--enable-sodium)

one optional to set the library directory (--with-libsodium=DIR)

And for consistency all "libsodium" fonction (minit, minfo, ...) should also be renamed

I don't think there should be --enable-sodium as it depends on the library. From what I know, enable is just for extensions without lib dependency and for lib dependency we always have --with or am I missing anything?

The extension is "sodium", and thus we should use some --with-sodium to be consistent, (enable/with is not revelant here)

See ext/zip for ex, it have "--enable-zip" for the extension and "--with-libzip" for the library.

Sure that's what I meant. I was just replying in regards to the one of the options that you suggested. Basically it shouldn't be enable but with. In this case --with-sodium ofc.

bukka · 2017-06-08T11:47:43Z

ext/sodium/LICENSE

@@ -0,0 +1,23 @@
+Copyright (c) 2013-2017, Frank Denis


Shouldn't all core extension be under the PHP license? I know that there are some files under other licenses but is there actually a core extension with other primary license than the PHP license. If this would be the first one I think that this would be good to mention on the internal list unless it's been already discussed and I just missed it...

bukka · 2017-06-08T11:48:25Z

ext/sodium/README.md

+[libsodium](https://github.com/jedisct1/libsodium).
+
+Full documentation here:
+[Using Libsodium in PHP Projects](https://paragonie.com/book/pecl-libsodium),


Are there any plans to move docs to PHP?

Yes. That would be a very good idea.

bukka · 2017-06-08T11:51:01Z

ext/sodium/libsodium.c

+
+PHP_FUNCTION(sodium_memzero)
+{
+	zval	  *buf_zv;


could the tab between tab and var be replaced with a single space to keep it more or less consistent with other exts.

I wish spaces could be used instead of having to mix tab and spaces.

This is not mixing of spaces and tabs. Indentation is always with tabs and alignment with spaces, otherwise it is impossible to ensure consistent formatting across different tab configurations. You are already encountering this effect here on GitHub. Tabs are the best choice for indentation, because they allow configuration, but that is exactly not what you want when you carefully align code for readability, hence spaces are the best here. 😺

@jedisct1 Personally I prefer spaces for indentation. However much more important thing is to keep the coding style consistent. In most extensions and core, there is just a single spaces...

bukka · 2017-06-08T11:52:42Z

ext/sodium/libsodium.c

+	zval	  *buf_zv;
+
+	if (zend_parse_parameters(ZEND_NUM_ARGS(),
+							  "z", &buf_zv) == FAILURE) {


wouldn't be better to indent it with less tabs? I think that 2 tabs (3 with the function indent) should be enough.

weltling · 2017-06-08T15:26:52Z

ext/sodium/config.w32

+// $Id$
+// vim:ft=javascript
+
+ARG_WITH("libsodium", "for libsodium support", "no");


The ext name should be adjusted here, too. In further also the variable would be PHP_SODIUM. I'm going to add the library builds to the windows deps package already.

Thanks.

remicollet · 2017-06-12T07:56:39Z

=> remicollet@167bbb6

@weltling please check config.w32

BTW, I agree on changing license to PHP one, @paragonie-scott what do you think ?

jedisct1 · 2017-06-12T09:19:06Z

Is changing the license a requirement?

I'd like to stick to the BSD license, if compatible with PHP requirements. Or else, have it dual licensed.

remicollet · 2017-06-12T09:52:37Z

PHP License is a BSD License, only changed to add the PHP name protection clause.

remicollet · 2017-06-12T10:11:49Z

Is changing the license a requirement?

Probably no, but I have no real answer, can you please raise this on Internals ?

remicollet · 2017-06-12T11:11:46Z

@paragonie-scott @jedisct1 BTW, if you are OK with my changes, I will merge it soon (at least, I'd like to see this before alpha2)

remicollet · 2017-06-13T06:24:37Z

=> http://news.php.net/php.internals/99503

remicollet · 2017-06-15T08:03:29Z

@jedisct1 @paragonie-scott ping

Fleshgrinder

Formatting is inconsistent, often off (hint: tab for indentation and spaces for alignment), and not optimized for readability.
Exception messages are inconsistent and mostly not helpful for the user.
Many optional constants and functions, library providers can never be sure what is available and what isn't. This leads to many defined and function_exists checks and pollutes user-level code. I don't know why this is like that, I guess it depends on the sodium library version. In that case it seems better to define a minimum version, rather than having optional features.

PS: Function names are hard to understand (e.g. scalarmult, kx). I know that this is done to be a direct copy of the underlying library. I still believe that this is wrong.

Fleshgrinder · 2017-06-15T09:29:49Z

ext/sodium/libsodium.c

+zend_module_entry libsodium_module_entry = {
+#if ZEND_MODULE_API_NO >= 20010901
+	STANDARD_MODULE_HEADER,
+#endif


Checking for the module API here seems unnecessary, this is shipped with the latest PHP only.

Fleshgrinder · 2017-06-15T09:29:54Z

ext/sodium/libsodium.c

+
+#ifndef PHP_FE_END
+# define PHP_FE_END { NULL, NULL, NULL }
+#endif


This seems unnecessary as it should always be defined.

Fleshgrinder · 2017-06-15T09:29:56Z

ext/sodium/libsodium.c

+	/* optional */
+	ZEND_ARG_INFO(0, key)
+	ZEND_ARG_INFO(0, length)
+ZEND_END_ARG_INFO()


Would be great if the argument info declarations are close to their respective functions. I also believe that it would be better to use type constraints, instead of just taking mixed types.

Fleshgrinder · 2017-06-15T09:30:03Z

ext/sodium/libsodium.c

+#include <sodium.h>
+#include <stdint.h>
+
+#define ZSTR_TRUNCATE(zs, len) do { ZSTR_LEN(zs) = (len); } while(0)


I don't think that it is a good idea to define a (global) Zend macro in an extension. This should rather be PHP_SODIUM_ZSTR_TRUNCATE, or moved to the other Zend macros.

Having that moved to other Zend macros would make sense, as it can be useful to other extensions.

Notice, we have zend_string_truncate

Fleshgrinder · 2017-06-15T09:30:05Z

ext/sodium/libsodium.c

+static zend_class_entry *sodium_exception_ce;
+
+const int pass_rest_by_reference = 1;
+const int pass_arg_by_reference = 0;


Constants should be SCREAMING_SNAKE_CASE and probably static too.

I think these are not used any more and could be dropped.

Fleshgrinder · 2017-06-15T09:33:22Z

ext/sodium/php_libsodium.h

+PHP_FUNCTION(sodium_increment);
+PHP_FUNCTION(sodium_add);
+PHP_FUNCTION(sodium_memcmp);
+PHP_FUNCTION(sodium_memzero);


Defining the functions in the header is bad practice.

Fleshgrinder · 2017-06-15T09:33:24Z

ext/sodium/php_libsodium.h

+
+#define crypto_kx_BYTES crypto_scalarmult_BYTES
+#define crypto_kx_PUBLICKEYBYTES crypto_scalarmult_SCALARBYTES
+#define crypto_kx_SECRETKEYBYTES crypto_scalarmult_SCALARBYTES


These should be prefixed with PHP_SODIUM_ and not crypto_kx_.

That would be pretty ugly and inconsistent.

These constants are defined in sodium >= 1.0.12, along with a better API so maybe the current crypto_kx function should be replaced with bindings to the sodium crypto_kx API.

Imho its inconsistent that these things are called crypto_ and not sodium_ from the library, but that is a different story. If that prefix is used by the library its fine.

Macros are always global, that is why it is important to have a clear naming scheme. In PHP that naming scheme is PHP_${EXT_NAME}_*.

Fleshgrinder · 2017-06-15T09:35:52Z

ext/sodium/libsodium.c

@@ -0,0 +1,2682 @@
+


PHP license header?

Fleshgrinder · 2017-06-15T09:40:46Z

ext/sodium/libsodium.c

+								   (unsigned long long) ciphertext_len,
+								   nonce, key) != 0) {
+		zend_string_free(msg);
+		RETURN_FALSE;


Why not an exception at this point?

Same here. Not a bug, likely not something the developers has to fix.

Same as above, not a bug, but your function failed and should communicate that. Exceptions have nothing to do with bugs.

Fleshgrinder · 2017-06-15T09:40:53Z

ext/sodium/libsodium.c

+		return;
+	}
+	if (ciphertext_len < crypto_secretbox_MACBYTES) {
+		RETURN_FALSE;


Why not an exception at this point?

It's not a misuse.

Some possibly user-supplied data cannot be decrypted. That's normal, and should probably not trigger an exception, just like strcmp() doesn't throw an exception if the strings don't match.

strcmp is a bad example, as it does not use exceptions in the first place, and opts for error codes. You are using exceptions on the other hand.

Exceptions are not meant to point out something that requires fixing, in that case an error is asked for (or even a fatal error, depending on the severity). Exceptions indicate that your routine was not able to perform its duty and uphold its promise, in this case decoding something. The caller can catch that exception and try to recover, according to her domain.

Your current API violates this and requires double error handling:

try { if (sodium_crypto_secretbox_open('foo', 'bar', 'baz') === false) { // could not open, handle problem ... } else { // successfully opened, continue flow ... } } catch (SodiumException $e) { // Actually an InvalidArgumentException ... }

You might ask now why InvalidArgumentException is an exception and not an error. We cannot tell whether it was a user-level developer mistake or end-user supplied data that lead to the problem, hence, we throw an exception which allows the user-level developer to take appropriate action. This could be an error message that is displayed to the end-user.

This could be an error message that is displayed to the end-user. that's exactly what applications using crypto should avoid.

The double error handling looks fine to me. They cover very different cases. The former is not an error. And most people will not even try to catch the exception, which is fine as well, as it will only be thrown on misuse.

For future bikeshedding purposes: s/should/must/?

@paragonie-scott this is in informal discussion and not a standard. People should understand that every comment expresses a personal opinion and not the law.

@jedisct1 once more, the invalid arguments should result in an InvalidArgumentException and not a SodiumException, as the latter would indicate that sodium was unable to perform its duty.

Returning false is fine if you guys prefer a procedural interface, but in that case all functions should do so consistently. This would mean that the SodiumException is deleted and proper exceptions from SPL are used that convey the desired meaning:

InvalidArgumentException for invalid arguments

LengthException for the various length related stuff

Error for the various unknown internal error things

OverflowException/UnderflowException for the arithmetic problems

The SodiumException as it stands now has no meaning, as it is a catch-all for any- and everything related to the sodium extension without conveying any meaning.

weltling · 2017-06-15T10:39:48Z

@remicollet

if (CHECK_LIB("libsodium.lib", "libsodium", PHP_LIBSODIUM) && CHECK_HEADER_ADD_INCLUDE("sodium.h", "CFLAGS_LIBSODIUM"))

still PHP_SODIUM and "CFLAGS_SODIUM" it should be. If i haven't overseen something, it should be it. But otherwise, probably would make sense to check Appveyor as well. I'll be checking after it's merged, but it might be quite short before tag.

Thanks.

nikic · 2017-06-15T11:02:56Z

@Fleshgrinder Thanks for the review. Just a procedural note to avoid confusion: We'd like to get this merged first and then perform any necessary cleanup or improvements after that, otherwise this is going to hang around forever. This PR should only deal with the remaining build system issues and not try to do any larger code changes.

Fleshgrinder · 2017-06-15T11:44:33Z

@nikic thanks for the heads up. In that case my remarks should be disregarded. I am happy to help with these smaller things, as some changes (e.g. type constraints) must be performed before the final release of 7.2 or they will result in a BC discussion.

remicollet · 2017-06-16T13:15:56Z

Rather (commits squashed/ammended) remicollet@a784069

@paragonie-scott can you pull above commit into this PR ?

nikic · 2017-06-16T13:16:16Z

@remicollet I've added you to the PHP organization, so you should be able to directly push to PRs in the future.

remicollet · 2017-06-16T13:20:17Z

@nikic thanks, will be very helpful

paragonie-scott · 2017-06-16T16:47:37Z

@remicollet I got a lot of merge conflicts when I tried to pull your changes in. Would your rather send a superseding PR?

remicollet · 2017-06-16T18:15:49Z

Im AFK for the WE

remicollet · 2017-06-19T05:47:50Z

@paragonie-scott have you tried to merge my branch ? or only the single commit ?
The second should work...

wget https://github.com/remicollet/php-src/commit/a78406954f06ebc84cea396f20842f201ce8997d.patch
git am a78406954f06ebc84cea396f20842f201ce8997d.patch

nikic · 2017-06-23T11:37:53Z

I've pushed remi's changes to the PR.

jhdxr · 2017-06-29T10:23:21Z

ext/sodium/libsodium.c

+		return;
+	}
+	if (len1 != len2) {
+		zend_throw_exception(sodium_exception_ce, "memcmp(): arguments have different sizes", 0);


I think the message should be "memcmp(): arguments should have same size", since we tell users what we except, like other error messages in this file.

sgolemon · 2017-06-29T23:52:36Z

@jedisct1 I'm not sure how looped into the conversation you've been, I've been using @paragonie-scott as my go-between because I wasn't clear who owned ext-sodium, but the LICENSE issue is probably our main blockers at this point. Anything to do with fixing up the code can be addressed post-merge, but pulling in an extension that's not under the PHP license would be unprecedented, so we'd like if you could re-license the extension to the PHP license.

If that's a blocker for you, we can potentially take the BSD license as-is, but to quote part of the internals conversation on the topic:

"""We copy and paste code between extensions. If different extensions use different copyrights doing this "correctly" becomes complicated. If all PHP-related code is using PHP License and copyright by The PHP Group this becomes easier."""

Naturally, this would not impact the license of libsodium itself, and you would retain your place in the CREDITS file.

nikic · 2017-06-30T00:33:36Z

@sgolemon As far as I'm aware relicensing of code is always tricky business requiring individual sign-off from all contributors. And what is being suggested here seems to go beyond a simple licensing change -- your quote also implies a transferal of copyright to the PHP group. I don't think that's a reasonable requirement, and it seems like even sketchier legal ground.

sgolemon · 2017-06-30T02:42:04Z

https://github.com/jedisct1/libsodium-php/graphs/contributors has 13 contributors. 7 of these are trivial contributions. Of the remaining 5 (not counting @jedisct1), it's: You, @paragonie-scott, and @remicollet who are all trivial to get sign-off from, leaving @xor-zz and @wlejon to track down. That's pretty minimal.

Do you have a better solution?

wlejon · 2017-06-30T07:02:55Z

the work I did was also trivial - but if you need something from me, I'm happy to provide.

jedisct1 · 2017-06-30T13:30:13Z

I guess we don't have any choice to have that code merged. The PHP license can be added to the current one so that the code is dual licensed.

sgolemon · 2017-06-30T13:59:56Z

@jedisct1 As I said, """If that's a blocker for you, we can potentially take the BSD license as-is""". The potential part of that means you're going to need to be present and responsive in the conversation, however. First, that means reading (which you don't seem to have) and responding (which you've done little of). @remicollet asked you to raise this issue on internals@ weeks ago and you neither responded to that here, nor raised the issue there.

Am I being a jerk about it right now? Yes. Because this RFC has been approved since February, yet at this rate we're going to end up hitting feature freeze without a merge because even the code review issues are being responded to in glacial time. I'm willing to clean the code up after merge, but licensing is important, and I'm not going to touch this code without a solid answer on licensing that protects the rest of php-src.

I'll translate your answer above as, "No, I refuse to change the license to PHP" which again, is fine. You also state you're willing to amend/qualify the license in some way, that's also doable. Unfortunately, I'm not a lawyer, and I'm also not the final authority for PHP. For both these reasons, this conversation needs to happen on the internals list.

remicollet · 2017-06-30T14:18:29Z

For memory http://news.php.net/php.internals/99503

sgolemon · 2017-06-30T14:24:58Z

Or with threading for the whole picture: https://externals.io/thread/945 ;)

But again, please don't post responses here. Post them on internals@. If you post them here, then someone has to repeat your arguments there and that will just slow things down.

jedisct1 · 2017-07-01T20:00:14Z

Ok, how do we change the license then?

sgolemon · 2017-07-02T14:50:05Z

But again, please don't post responses here. Post them on internals@. If you post them here, then someone has to repeat your arguments there and that will just slow things down.

paragonie-scott · 2017-07-07T17:11:26Z

We have unanimous consent from everyone who has contributed any changes to that repository, so let's make it happen.

remicollet · 2017-07-09T17:01:39Z

@paragonie-scott can you please add the standard license headers and squash the commit ?

paragonie-scott · 2017-07-10T16:19:06Z

@remicollet Done and done.

weltling · 2017-07-10T16:49:41Z

@paragonie-scott here at least https://github.com/php/php-src/pull/2560/files#diff-5597f3b22c08f00df6221764b50ea1ebR7 the ext name is still "libsodium".

Thanks.

paragonie-scott · 2017-07-10T16:50:20Z

Okay, will fix that ASAP.

Can we make all future corrections after it's merged?

RFC: https://wiki.php.net/rfc/libsodium Licensing: https://web.archive.org/web/20170710161517/https://github.com/jedisct1/libsodium-php/issues/127

weltling · 2017-07-10T16:53:50Z

I don't mind, only mentioned this as it doesn't even compile on AppVeyor. No bikeshedding here :) I could fix it after it's merged as well.

Thanks.

remicollet · 2017-07-11T05:31:14Z

Thanks for you patience.

Merged

remicollet · 2017-07-11T05:31:44Z

BTW, can you check the "Authors" line in the header, I don't think this is correct

remicollet · 2017-07-11T05:35:35Z

Hmm.. travis failure
E: Unable to locate package libsodium-dev

Despite seems to exist, https://packages.debian.org/search?keywords=libsodium&searchon=names

remicollet · 2017-07-11T05:44:59Z

Please check d269aa2
@paragonie-scott @jedisct1

remicollet reviewed Jun 6, 2017

View reviewed changes

bukka reviewed Jun 8, 2017

View reviewed changes

weltling reviewed Jun 8, 2017

View reviewed changes

LinuzLuzas mentioned this pull request Jun 9, 2017

Php 7.0 #2571

Closed

krakjoe added RFC Waiting on Author labels Jun 13, 2017

Fleshgrinder reviewed Jun 15, 2017

View reviewed changes

jhdxr reviewed Jun 29, 2017

View reviewed changes

paragonie-scott force-pushed the sodium branch from c2eed87 to a42b20c Compare July 10, 2017 16:18

paragonie-scott force-pushed the sodium branch from a42b20c to cab8d1a Compare July 10, 2017 16:20

Squashed commit - Add ext/sodium

3ec2d51

RFC: https://wiki.php.net/rfc/libsodium Licensing: https://web.archive.org/web/20170710161517/https://github.com/jedisct1/libsodium-php/issues/127

paragonie-scott force-pushed the sodium branch from cab8d1a to 3ec2d51 Compare July 10, 2017 16:51

remicollet closed this Jul 11, 2017

glensc mentioned this pull request Jul 21, 2017

[Meta] PHP 7.2 features PHP-CS-Fixer/PHP-CS-Fixer#2907

Closed

7 tasks


		PHP_SUBST(LIBSODIUM_SHARED_LIBADD)

		PHP_NEW_EXTENSION(libsodium, libsodium.c, $ext_shared)

Add ext/sodium for PHP 7.2.0 #2560

Add ext/sodium for PHP 7.2.0 #2560

Uh oh!

Conversation

paragonie-scott commented Jun 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paragonie-scott Jun 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

remicollet commented Jun 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

remicollet commented Jun 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jedisct1 commented Jun 12, 2017

Uh oh!

remicollet commented Jun 12, 2017

Uh oh!

remicollet commented Jun 12, 2017

Uh oh!

remicollet commented Jun 12, 2017

Uh oh!

remicollet commented Jun 13, 2017

Uh oh!

remicollet commented Jun 15, 2017

Uh oh!

Fleshgrinder left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fleshgrinder Jun 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

paragonie-scott Jun 6, 2017 •

edited

Loading

remicollet commented Jun 12, 2017 •

edited

Loading

Fleshgrinder left a comment •

edited

Loading

Fleshgrinder Jun 15, 2017 •

edited

Loading

Fleshgrinder Jun 15, 2017 •

edited

Loading