PHPC-888: Support additional TLS libraries for bundled libmongoc #736

jmikola · 2018-01-10T17:44:36Z

https://jira.mongodb.org/browse/PHPC-888
https://jira.mongodb.org/browse/PHPC-1003

jmikola · 2018-01-13T07:26:07Z

scripts/build/autotools/CheckSSL.m4

+  found_libressl="no"
+
+  dnl TODO: this may require adding libcrypto to the modules list, as pkg-config does not seem to include it (unlike openssl)
+  PKG_CHECK_MODULES([PHP_MONGODB_SSL],[libtls],[


Note: when testing this on my Ubuntu system, I actually had to change "libtls" to "libressl-tls" (since I'm using a custom PPA). I also noticed that pkg-config --libs libressl-tls returns only -lressl-tls, unlike pkg-config --libs openssl, which returns -lssl -lcrypto. I'm not sure if that's specific to the PPA package I'm using, so I'll have to test with LibreSSL proper.

FWIW, libmongoc also only specifics "libtls", so I'm curious if their LibreSSL builds are linking -lcrypto or not.

It's possible that your PPA doesn't need the -lcrypto, and just that -lressl-tls contains all of what -lssl and -lcrypt export. I would actually go as far to make sure that we can support your custom PPA, and the normal libressl.

derickr · 2018-01-15T10:27:11Z

config.m4

@@ -83,7 +79,7 @@ AC_DEFUN([PHP_BSON_CLOCK],
  fi
 ])

-if test "$MONGODB" != "no"; then
+if test "$PHP_MONGODB" != "no"; then


Heh - this must never have worked then.

No sir, it did not :)

derickr · 2018-01-15T10:28:15Z

scripts/build/autotools/CheckHost.m4

+        ;;
+    *-*-*netbsd*)
+        os_netbsd=yes
+	TARGET_OS=unix


There are some whitespace issues in this file.

It looks like the TARGET_OS assignments all used tab indents. Fixing now.

derickr · 2018-01-15T10:30:36Z

scripts/build/autotools/CheckSSL.m4

+PHP_ARG_WITH([mongodb-ssl],
+             [whether to enable crypto and TLS],
+             [AS_HELP_STRING([--with-mongodb-ssl=@<:@auto/no/openssl/libressl/darwin@:>@],
+                             [Enable TLS connections and SCRAM-SHA-1 authentication [default=auto]])],


I think all the help options, should be prefixed with " MongoDB: ", just as I currently do at https://github.com/xdebug/xdebug/blob/master/config.m4#L7

derickr · 2018-01-15T10:31:36Z

scripts/build/autotools/CheckSSL.m4

+    AC_CHECK_DECLS([ASN1_STRING_get0_data],
+                   [have_ASN1_STRING_get0_data="yes"],
+                   [have_ASN1_STRING_get0_data="no"],
+                   [[#include <openssl/asn1.h>]])


Why are there two sets of [ ] here?

Quoting https://lists.gnu.org/archive/html/bug-gnulib/2011-09/msg00172.html:

# is a comment character in m4, which reads until end-of-line to end the comment. Which means all the [] after the comment character are treated as part of the comment, instead of as m4 [] quoting.

https://lists.gnu.org/archive/html/bug-gnulib/2011-09/msg00193.html discusses modifying AC_CHECK_DECLS to double-quote.

Note that we also used double-quoting in the original check in config.m4.

derickr · 2018-01-15T10:33:32Z

scripts/build/autotools/CheckSSL.m4

+  found_openssl="no"
+
+  PKG_CHECK_MODULES([PHP_MONGODB_SSL],[openssl],[
+    dnl PKG_CHECK_MODULES() doesn't check for headers, so confirm they exist (mainly for macOS)


That seems odd. The whole idea behind pkg_config is that its --includes (or whatever the flag is), shows all the header locations. I don't think we should go check all the headers manually where pkg-config works.

derickr · 2018-01-15T10:35:00Z

scripts/build/autotools/CheckSSL.m4

+  found_libressl="no"
+
+  dnl TODO: this may require adding libcrypto to the modules list, as pkg-config does not seem to include it (unlike openssl)
+  PKG_CHECK_MODULES([PHP_MONGODB_SSL],[libtls],[


It's possible that your PPA doesn't need the -lcrypto, and just that -lressl-tls contains all of what -lssl and -lcrypt export. I would actually go as far to make sure that we can support your custom PPA, and the normal libressl.

derickr · 2018-01-15T10:36:14Z

scripts/build/autotools/CheckSSL.m4

+
+dnl Disable Windows SSL and crypto
+AC_SUBST(MONGOC_ENABLE_SSL_SECURE_CHANNEL, 0)
+AC_SUBST(MONGOC_ENABLE_CRYPTO_CNG, 0)


Is the idea to support these later?

I don't believe so, since Windows builds use config.w32 and will never use Autotools. We do still need to make the substitutions, though.

derickr · 2018-01-15T10:37:05Z

scripts/build/autotools/CheckSSL.m4

+
+  CFLAGS="$old_CFLAGS"
+else
+  AC_SUBST(MONGOC_HAVE_ASN1_STRING_GET0_DATA, 0)


You've already done the ASN1_STRING_get0_data check earlier. Do you need it twice?

I believe I forgot to remove this if/else block from the file. The problem with this is that CFLAGS may change depending on whether we found OpenSSL with pkg-config (sets PHP_MONGODB_SSL_CFLAGS) or manual header searching and PHP_CHECK_LIBRARY (sets OPENSSL_INCDIR).

Therefore, this if/else block should be removed and we should only keep the one below here:

if test "x$have_ASN1_STRING_get0_data" = "xyes"; then AC_SUBST(MONGOC_HAVE_ASN1_STRING_GET0_DATA, 1) else AC_SUBST(MONGOC_HAVE_ASN1_STRING_GET0_DATA, 0) fi

jmikola · 2018-01-17T17:41:23Z

@sibidharan: Since you did such a great job solving the BoringSSL issues, I wonder if you'd be willing to help us test this PR on macOS. I plan to add macOS to our Travis CI matrix, but would still appreciate a fresh set of eyes on it.

This PR introduces a new configure option, --with-mongodb-ssl=[auto/no/openssl/libressl/darwin]. Specifying --with-mongodb-ssl=darwin should allow you to build the driver with its bundled version of libbson and libmongoc and instruct libmongoc to use Secure Transport instead of OpenSSL. This will ultimately remove the need in your BoringSSL workaround to first install libbson and libmongoc as system libraries. Once compiled, you should also be able to verify that the driver is using its bundled libmongoc with Secure Transport via phpinfo() output.

sibidharan · 2018-01-18T00:33:52Z

That is indeed such a great news @jmikola

I will test it out on all our peer systems running on couple different versions of MacOS.

jmikola · 2018-01-19T02:52:32Z

@derickr: The last commit adds three macOS environments to the build matrix:

xcode8.3 with php56 and --with-mongodb-ssl=auto (OpenSSL)
xcode9.2 with php72 and --with-mongodb-ssl=auto (OpenSSL)
xcode9.2 with php72 and --with-mongodb-ssl=darwin (Secure Transport)

Note that since we only have a standalone mongod running on Travis, this is only testing that the driver builds successfully and can execute non-SSL code paths. We'll definitely want to do a manually test on a Mac later on.

I've no idea when these will actually complete, as Travis CI status is reporting degraded performance for macOS builds and their backlog appears to consistently be >1000 jobs.

derickr · 2018-01-19T11:44:37Z

.travis.scripts/before_script.sh

+make install
+
+# Use latest run-tests.php, as older versions don't report a non-zero exit code for failures
+wget -O run-tests.php https://raw.githubusercontent.com/php/php-src/master/run-tests.php


Do we still need to do this?

While I very much remember the problem with run-tests.php exiting with zero after failures, I don't quite remember what versions of PHP demonstrated the issue. There is evidence of exit(1) going back to PHP-5.3.

I did come across php/php-src@16efc77, which changed the logic for exit(1) in 5.5. Do you recall if this was only an issue up to 5.4 (inclusive)?

If you're able to narrow it down, we can certainly get rid of this.

I'll narrow this down in a bit and if 5.5+ reports exit codes, we can remove this.

Confirmed that PHP 5.4 was the last version with this problem. I'll remove this.

derickr · 2018-01-19T11:46:37Z

tests/connect/standalone-x509-extract_username-002.phpt

@@ -2,6 +2,7 @@
 Connect to MongoDB with SSL and X509 auth and username retrieved from cert (stream context)
 --SKIPIF--
 <?php require __DIR__ . "/../utils/basic-skipif.inc"; ?>
+<?php NEEDS_SSL(['OpenSSL', 'Secure Transport', 'Secure Channel']); ?>


Why can this not be LibreSSL?

See: https://github.com/mongodb/mongo-c-driver/blob/1.9.2/src/mongoc/mongoc-ssl.c#L78

derickr · 2018-01-19T11:46:57Z

scripts/build/autotools/m4/pkg.m4

@@ -0,0 +1,199 @@
+# pkg.m4 - Macros to locate and utilise pkg-config.            -*- Autoconf -*-
+# serial 1 (pkg-config-0.24)


This file isn't one you wrote yourself, right?

Correct. There may be whitespace issues (mixed tabs and spaces) that I can clean up, though. We imported from libmongoc, but they also pulled it from elsewhere.

derickr · 2018-01-19T11:48:09Z

config.m4

@@ -502,7 +477,7 @@ if test "$MONGODB" != "no"; then
      AC_OUTPUT($srcdir/src/libmongoc/src/zlib-1.2.11/zconf.h)
    fi
  fi
-  if test "$PHP_LIBBSON" == "no"; then
+  if test "$PHP_LIBBSON" = "no"; then


Was this always broken like this before? Or do both = and == work (on bash)?

Bash supports both == and =, but = is POSIX compliant. You'll note from the new shell scripts that I'm aiming to keep things portable with #!/bin/sh and POSIX-friendly syntax.

derickr · 2018-01-19T11:50:28Z

.travis.yml


 before_install:
-  - sudo pip install --upgrade cpp-coveralls
-  - sudo pip install ${MONGODB_ORCHESTRATION_REPO}


I can't see where mongo orchestration is installed through in the new code.

See ef72444d5239f43792a942cd9938b89bb4fbde1c. These were merely artifacts from when Hannes was using Mongo Orchestration locally to spin up test environments (PHPC-128). I found nothing in our current .travis.yml files that actually launched Mongo Orchestration, so this all appears to be dead code.

jmikola · 2018-01-19T16:19:53Z

@derickr: I borrowed a Macbook (macOS 10.12.5) and verified that I'm able to successfully compile the driver for Secure Transport using --with-mongodb-ssl=darwin. ./configure reports that MONGODB_SHARED_LIBADD includes -framework Security -framework CoreFoundation and phpinfo() reports "Secure Transport" and "Common Crypto". Additionally, I successfully connected to an Atlas cluster with SSL and executed a buildInfo command.

@sibidharan: If you're still able to test, that'd be appreciated; however, you're not holding us up :)

@alcaeus: I expect we'll cut a 1.4.0-RC1 release next week. Would it be possible to update the ext-mongodb formulae to start using --with-mongodb-ssl=darwin soon after?

The double equal operator is a Bash-specific alias

Travis' backlog for macOS builds is quite high, and the environments also do not support MongoDB. We can revisit adding these environments down the line when the infrastructure has improved and we sort out explicit installation of MongoDB versions (see: PHPLIB-299). Note: a macOS build with Secure Channel was manually tested in mongodb#736.

derickr

I've made a PR against yours with a few tweaks:
jmikola#2

Besides that, it looks good and things worked as expected on the platforms I've tried it on (various Debians)

derickr · 2018-01-22T10:50:16Z

scripts/build/autotools/CheckSSL.m4

+  fi
+])
+
+AS_IF([test "$PHP_MONGODB_SSL" = "darwin" -o \( "$PHP_MONGODB_SSL" = "auto" -a "$os_darwin" = "yes" \)],[


Something is going wrong here, as I can select "darwin" as option on my Linux system and configure doesn't complain. It will of course fail when trying to compile.

@bjori

This was originally added in dcd732c and should have been removed when @bjori's coverity hook was removed in 5309c17.

These lines were originally added before 1.0 when a local Mongo Orchestration process was used to create the test environments (see: PHPC-128).

If a directory is excluded, a negation on the files within it (e.g. "!dir/*") has no effect. Since .travis.scripts/ is excluded by ".*", we need to negate the directory itself to ensure the files within are tracked.

This was last updated in 90a64b4 when the project was hosted in the 10gen-labs groups.

PHP 5.4 was the last version where run-tests.php failed to report a non-zero exit code after a failure. Since the extension requires 5.5+, we can rely on any existing run-tests.php file.

Travis' backlog for macOS builds is quite high, and the environments also do not support MongoDB. We can revisit adding these environments down the line when the infrastructure has improved and we sort out explicit installation of MongoDB versions (see: PHPLIB-299). Note: a macOS build with Secure Channel was manually tested in mongodb#736.

This makes for more chronological configure output

This also documents "yes" values for the libbson/libmongoc parameters, and the "auto" default for SASL.

jmikola force-pushed the phpc-888 branch 5 times, most recently from fdd3f3e to 1017c75 Compare January 13, 2018 07:20

jmikola commented Jan 13, 2018

View reviewed changes

jmikola requested a review from derickr January 13, 2018 07:26

derickr reviewed Jan 15, 2018

View reviewed changes

jmikola force-pushed the phpc-888 branch 5 times, most recently from 8d3de91 to 4806b57 Compare January 17, 2018 00:33

jmikola force-pushed the phpc-888 branch 4 times, most recently from 8072f70 to b1a506a Compare January 19, 2018 02:43

derickr suggested changes Jan 19, 2018

View reviewed changes

jmikola force-pushed the phpc-888 branch 2 times, most recently from 41a5ef9 to 982de4b Compare January 19, 2018 15:15

jmikola force-pushed the phpc-888 branch from 982de4b to 88adea8 Compare January 19, 2018 18:36

jmikola changed the title ~~[WIP] PHPC-888: Support additional TLS libraries for bundled libmongoc~~ PHPC-888: Support additional TLS libraries for bundled libmongoc Jan 19, 2018

jmikola added 3 commits January 19, 2018 17:29

Do not ignore nested "build" directories

b1a6690

Fix conditional for whether extension is enabled

8af9175

Use single equal sign for test comparisons

274a444

The double equal operator is a Bash-specific alias

jmikola force-pushed the phpc-888 branch from 88adea8 to c504d8f Compare January 19, 2018 22:29

jmikola mentioned this pull request Jan 20, 2018

php: import from homebrew/php Homebrew/homebrew-core#16067

Closed

10 tasks

derickr approved these changes Jan 22, 2018

View reviewed changes

jmikola and others added 16 commits January 22, 2018 12:01

PHPC-888: Support additional TLS libraries for bundled libmongoc

d679bfe

Remove old API key for Coveralls

16fe17c

This was originally added in dcd732c and should have been removed when @bjori's coverity hook was removed in 5309c17.

Remove old Mongo Orchestration artifacts

0e686fd

These lines were originally added before 1.0 when a local Mongo Orchestration process was used to create the test environments (see: PHPC-128).

Do not ignore files within .travis.scripts/

28c717b

If a directory is excluded, a negation on the files within it (e.g. "!dir/*") has no effect. Since .travis.scripts/ is excluded by ".*", we need to negate the directory itself to ensure the files within are tracked.

LibreSSL cannot extract username subject from X509 certs

83f3330

Coveralls API token is not needed for public projects

61d4ffb

This was last updated in 90a64b4 when the project was hosted in the 10gen-labs groups.

Make core and diff debugging more portable

df9c278

Add macOS environments to build matrix

d4e998f

Upgrading run-tests.php is no longer necessary

071f90b

PHP 5.4 was the last version where run-tests.php failed to report a non-zero exit code after a failure. Since the extension requires 5.5+, we can rely on any existing run-tests.php file.

PHPC-1003: Use container environment for Travis builds

fe7ee68

Use AC_HELP_STRING for PHP_ARG_ENABLE in config.m4

0f8598f

PHPC-888: Make sure you can only select "darwin" on macOS

c2988bb

Move crypto system profile flags below OpenSSL detection

0002e2d

This makes for more chronological configure output

Document "no" defaults for PHP_ARG_ENABLE flags in config.m4

6f9bd6e

Use AC_HELP_STRING for PHP_ARG_WITH in config.m4

82516b3

This also documents "yes" values for the libbson/libmongoc parameters, and the "auto" default for SASL.

jmikola force-pushed the phpc-888 branch from c504d8f to 82516b3 Compare January 22, 2018 17:03

jmikola mentioned this pull request Jan 22, 2018

Config tweaks jmikola/mongo-php-driver#2

Closed

jmikola merged commit 82516b3 into mongodb:master Jan 22, 2018

jmikola added a commit that referenced this pull request Jan 22, 2018

Merge pull request #736

650335b

jmikola deleted the phpc-888 branch January 22, 2018 17:27

This was referenced Jan 23, 2018

Simplify Matrix #743

Merged

Compile error with LibreSSL #507

Closed

jmikola mentioned this pull request Dec 20, 2019

Fix logic for OS check #1076

Merged

		@@ -0,0 +1,199 @@
		# pkg.m4 - Macros to locate and utilise pkg-config. -- Autoconf --
		# serial 1 (pkg-config-0.24)

PHPC-888: Support additional TLS libraries for bundled libmongoc #736

PHPC-888: Support additional TLS libraries for bundled libmongoc #736

Uh oh!

Conversation

jmikola commented Jan 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

jmikola Jan 15, 2018 • 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!

jmikola commented Jan 17, 2018

Uh oh!

sibidharan commented Jan 18, 2018

Uh oh!

jmikola commented Jan 19, 2018

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!

jmikola commented Jan 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

derickr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jmikola commented Jan 10, 2018 •

edited

Loading

jmikola Jan 15, 2018 •

edited

Loading

jmikola commented Jan 19, 2018 •

edited

Loading