CMake: make HTTPS support more generic by tiennou · Pull Request #4283 · libgit2/libgit2

tiennou · 2017-06-23T22:51:28Z

Extracted from #4173 and rebased on master, switched to the new HTTPS nomenclature while keeping most of the functional changes.

tiennou · 2017-06-23T22:54:39Z

@pks-t Here it is ! I'll rebase current #4173 on top of that one while I'm at it.

pks-t · 2017-06-24T07:47:43Z

Thanks a lot! Will review in the near future.

pks-t

Very nice PR indeed, I definitly like what's going on here. There are some adjustments needed here, would be nice if you could make them.

Anyway, thanks for working on this!

pks-t · 2017-06-24T15:25:05Z

+	if (res < 0) {
+		return res;
+	}
+


An error message would be nice here to aid the user.

pks-t · 2017-06-24T15:27:00Z

 #include "filter.h"
 #include "merge_driver.h"
-#include "openssl_stream.h"
+#include "streams/openssl.h"


Just putting it at an arbitratry position: you should probably adjust the include guards of renamed header files, even though it's mostly a cosmetic change

pks-t · 2017-06-24T15:29:55Z

+int git_openssl_set_cert_file(const char *file, const char *path)
+{
+	if (SSL_CTX_load_verify_locations(git__ssl_ctx, file, path) == 0) {
+		giterr_set(GITERR_SSL, "SSL error: %s",


Maybe be a bit more specific here? E.g. "could not set default locations for certificates"?

Yep, fixed. I also tweaked some error codes where those functions are called.

pks-t · 2017-06-24T15:30:34Z

 	return error;
 }

+int git_openssl_set_cert_file(const char *file, const char *path)


We usually use "__" to denote private API. In this case "git_openssl__set_cert_file".

pks-t · 2017-06-24T15:31:21Z


 extern int git_openssl_stream_new(git_stream **out, const char *host, const char *port);

+extern int git_openssl_set_cert_file(const char *file, const char *path);


Some documentation would be nice here, even if it's private API. I certainly would be confused what "file" and "path" actually mean in this context

pks-t · 2017-06-24T15:32:28Z

 	return -1;
 }

+int git_openssl_set_ca_location(const char *file, const char *path)


Don't you mean git_openssl_set_cert_file here?

Yep, copy-paste failure. Settled on git_openssl__set_cert_location.

pks-t · 2017-06-24T15:35:06Z

 ENDIF()

+IF (USE_HTTPS STREQUAL "ON")
+	IF (SECURITY_FOUND)


It's a bit strange that we're actually searching for the security framewok even if USE_HTTPS=OFF here. But we can improve that later, no need to do it as part of this PR

Fixed, USE_TLS=OFF should disable all crypto-checking we do.

pks-t · 2017-06-24T15:36:08Z

+
+IF (HTTPS_BACKEND)
+	ADD_DEFINITIONS(-DGIT_HTTPS)
+ENDIF()


Seeing all this code we could even think about making this a separate CMake module later on. Not required for this PR, but I'll tackle this at some point, I guess.

Yeah, that's what I was thinking too, now that the backend selection is in one place 😉.

pks-t · 2017-06-24T15:38:33Z

 	ADD_DEFINITIONS(-DGIT_SHA1_WIN32)
 	FILE(GLOB SRC_SHA1 src/hash/hash_win32.c)
-ELSEIF (${CMAKE_SYSTEM_NAME} MATCHES "Darwin")
+ELSEIF (TLS_BACKEND MATCHES "SecureTransport")


This should be HTTPS_BACKEND now, shouldn't it? I guess this stems from the fact that I've been using HTTPS with the GIT_HTTPS define. But actually, GIT_TLS and TLS_BACKEND are nicer names here. So we should certainly be sticking with one name -- I don't care if you want to rename existing usage of GIT_HTTPS to GIT_TLS or if you'll just stick with GIT_HTTPS for now.

"Reverted" patchset to GIT_TLS then.

pks-t · 2017-06-24T15:38:39Z

+ELSEIF (TLS_BACKEND MATCHES "SecureTransport")
 	ADD_DEFINITIONS(-DGIT_SHA1_COMMON_CRYPTO)
-ELSEIF (OPENSSL_FOUND)
+ELSEIF (TLS_BACKEND MATCHES "OpenSSL")


Same as above.

pks-t · 2017-06-24T15:41:23Z

Ah, one additional comment: there are some commit messages without a scope (e.g. "cmake:") in their header. Could you maybe add one where it's missing? :)

tiennou · 2017-07-01T20:41:48Z

@pks-t Since you've proposed #4282, I'm somewhat wary of touching CMakeLists.txt, so if you don't mind I'll wait until it's merged. I'll go test #4282 though, if that helps.

pks-t · 2017-07-03T07:52:02Z

Well, I certainly don't mind at all. It's a bit unfortunate that #4282 is so invasive and clashes with all other PRs touching CMakeLists.txt.

We could move forward your PR and I'll try to fix merge conflicts in #4282. By now I've solved enough merge conflicts in there to know the exact steps required to reconstruct the final move, and the other commits are quite localized and as such easy to rebase. Just state your preference and I'll try to adapt accordingly.

If you want to move this one forward I'll try to focus on getting it ready and merged such that I can include these changes as soon as possible.

tiennou · 2017-07-03T08:49:52Z

As it stands, I feel this one is ready. I rebased it just yesterday.

pks-t · 2017-07-03T13:02:27Z

 ENDIF()

 IF (SECURITY_FOUND)
-  ADD_DEFINITIONS(-DGIT_SECURE_TRANSPORT)


This commit does a bit more than its title states, as it also removes the definitions for platform-specific TLS specs. There are additional uses of these defines which you haven't covered in this commit, though:

GIT_SECURE_TRANSPORT is used in stransport_stream.c and tls_stream.c

GIT_OPENSSL is used in global.h, netops.h, openssl_stream.{c,h}

This is just an indentation fixup. The defines are still here (line +2).

Ah, sorry. I've been confused by the missing -DGIT_HTTPS.

pks-t · 2017-07-03T13:07:13Z

 	SET( USE_ICONV ON )
-	FIND_PACKAGE(Security)
+	IF(USE_TLS STREQUAL "OpenSSL" OR USE_TLS STREQUAL "ON")
+		FIND_PACKAGE(Security)


Shouldn't this be IF(USE_TLS STREQUAL "SecureTransport" OR USE_TLS STREQUAL "ON")?

Will fix. I'll also check how this reacts to the CommonCrypto hash backend, because I think that checking for Security encompasses CommonCrypto & SecureTransport, so the hash backend might be broken by this change.

pks-t · 2017-07-03T13:14:03Z

 	ADD_DEFINITIONS(-DGIT_SHA1_WIN32)
 	FILE(GLOB SRC_SHA1 src/hash/hash_win32.c)
-ELSEIF (${CMAKE_SYSTEM_NAME} MATCHES "Darwin")
+ELSEIF (TLS_BACKEND MATCHES "SecureTransport")


This looks like it should use ${TLS_BACKEND}, shouldn't it? Shouldn't this use STREQUAL, as all previous code? Same below for OpenSSL.

IIRC CMake original behavior was to always substitute variables in some cases, IF being one of them.

I'll switch to STREQUAL though, as TLS_BACKEND is supposed to be an internal option.

Also, after taking a closer look, this line makes the hash-selection dependent on the backend. Old behavior was "always use CommonCrypto" on macOS, now it's "choose whatever the selected TLS backend provides, except when using SHA1DC". Is that okay ?

Huh, that's right. I think it would make more sense to stick with the DARWIN check here instead. Otherwise, it's becoming hard (impossible?) to use the CommonCrypto SHA1 library when TLS is turned off.

tiennou · 2017-07-07T11:20:30Z

I discovered some issues with it (on top of the CommonCrypto thing), so I vote for deferring this one after #4282.

tiennou · 2017-07-07T11:22:00Z

+IF(USE_TLS STREQUAL "ON")
+	IF (SECURITY_FOUND)
+		# OS X 10.7 and older do not have some functions we use, fall back to OpenSSL there
+		CHECK_LIBRARY_EXISTS("${SECURITY_DIRS}" SSLCreateContext "Security/SecureTransport.h" HAVE_NEWER_SECURITY)


Aforementioned issue. Forcing the SecureTransport backend will skip this test.

pks-t · 2017-07-07T15:19:30Z

Okay, as you wish. Only makes my own life easier :p

Thanks for keeping up the work on this!

tiennou · 2017-08-09T22:14:13Z

Rebased.

I added some commits before tackling the USE_TLS option so the backend selection wouldn't be so messy.

cc50581 moves the "Security too old" check in our module.
33aa5c4 adds FeatureSummary support (fixes Have CMake show a summary of the build options/decisions #3200, because I was using that to check what CMake was doing)
note that we now unconditionally check for everything (eg. OpenSSL/Security/whatever), so the backend selection code can decide what's best.

Something I'm still unsold on is the ability to prefer OpenSSL on macOS ; since we are always choosing CommonCrypto for SHA here and nobody has ever complained about not being able to not link against it, I'll keep it that way (but the fix is easy enough if needed).

pks-t · 2017-08-26T09:59:08Z

#4282 has landed now, so we can actually continue working on this PR here.

tiennou · 2017-08-29T00:32:44Z

Rebased and ready to go ! I've reverted the HTTPS->TLS change (again), because I think it predates 0.26 and I didn't want to break things.

pks-t

Thanks for rebasing! Here's another review. I guess this should be the last round of reviews from my side. I really look forward to merging it as it's much cleaner than what we currently have.

pks-t · 2017-09-01T05:37:14Z

@@ -9,8 +9,8 @@



Is the "tls_stream" file special in any way? I'd have expected it to be moved to the "streams" directory, as well.

Will fix, I remember some conflict when rebasing, which I obviously mishandled.

pks-t · 2017-09-01T05:43:32Z

+			MESSAGE("-- Found CoreFoundation ${COREFOUNDATION_LIBRARIES}")
+		ENDIF()
+		SET(COREFOUNDATION_FOUND TRUE)
+		SET(COREFOUNDATION_LDFLAGS "-framework CoreFoundation")


The LDFLAGS variable may not be set in the first case, where you only check whether COREFOUNDATION_INCLUDE_DIR and COREFOUNDATION_LIBRARIES are set. If you touch this place again, I'd love if you could split this up into two patches: the first one fixes indentation only and the second one introduces your changes. This would make it a bit easier to review.

I'll add a reindent commit before then.

About those changes, what I'm attempting to do is stopping the cmake run from echoing

-- Found Security /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/Frameworks/Security.framework -- Found CoreFoundation /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/Frameworks/CoreFoundation.framework

on every run. Since after the first run, everything gets cached, so this just short-circuit the test. I can also make both FIND_PACKAGE(... QUIET REQUIRED), but this silences it completely...

I ended up removing my bad attempt at using the cache correctly.

pks-t · 2017-09-01T05:44:50Z

+			MESSAGE("-- Found Security ${SECURITY_LIBRARIES}")
+		ENDIF()
+		SET(SECURITY_FOUND TRUE)
+		SET(SECURITY_LDFLAGS "-framework Security")


Same as for the other module changes. Here, both SECURITY_LDFLAGS and SECURITY_HAS_SSLCREATECONTEXT may be missing in the first case.

pks-t · 2017-09-01T05:45:44Z

+	ELSE()
+		MESSAGE("-- Security framework is too old, falling back to OpenSSL")
+		SET(USE_OPENSSL "ON")
+	ENDIF()


Nice to see that the main CMakeLists.txt becomes easier to read :)

pks-t · 2017-09-01T05:46:10Z

 OPTION( DEBUG_POOL			"Enable debug pool allocator"			OFF )
 OPTION( ENABLE_WERROR			"Enable compilation with -Werror"		OFF )

+


Random newline here

pks-t · 2017-09-01T05:48:59Z

 OPTION( USE_SHA1DC			"Use SHA-1 with collision detection"	OFF )
 OPTION( USE_ICONV			"Link with and use iconv library" 		OFF )
 OPTION( USE_SSH				"Link with libssh to enable SSH support" ON )
+OPTION( USE_HTTPS			"Enable HTTPS support"					ON )


Maybe mention that this is not a boolean only but instead can also be a specific backend?

pks-t · 2017-09-01T05:53:11Z

+ENDIF()
+
+IF(USE_HTTPS AND NOT HTTPS_BACKEND)
+	MESSAGE(FATAL_ERROR "Asked for backend ${HTTPS_BACKEND} but it wasn't found")


I think this cannot happen. If USE_HTTPS is set, then we'll always set it to a value with "OpenSSL" being the fallback. So in case of unknown backends we'll always fail with "OpenSSL not found" above, which is unfortunate.

Actually, it should trigger when manually asking for a backend we don't support (eg. -DUSE_HTTPS=gnuTLS). Otherwise, with -DUSE_HTTPS=ON (the default) the automatic detection will auto-select OpenSSL, which will hard-fail if we can't find OpenSSL (which is what we want, since we can't obey USE_HTTPS).

I'm confused now... I'll try to come up with a cleaner set of IF blocks then.

tiennou · 2017-09-06T23:08:03Z

Rebased, and added a commit to not link against CoreFoundation when we don't use the SecureTransport backend.

Also, I feel like I should tell that CMake's OpenSSL module doesn't work against Homebrew - it ends up linking with /usr/lib/libssl.dylib /usr/lib/libcrypto.dylib, which are the system-supplied versions (0.9.8z IIRC).

pks-t

From my side, I feel like it is mostly ready to go.

Regarding the Homebrew OpenSSL stuff. Is this something that's being broken by this PR or was it broken before already? If it's the second, I guess we can ignore that for now and fix it in another PR to finally get your changes inside.

pks-t · 2017-09-22T07:59:17Z

+MARK_AS_ADVANCED(
+	COREFOUNDATION_INCLUDE_DIR
+	COREFOUNDATION_LIBRARIES
+)


Do we really want to mark those as advanced? I usually see this being used only for internal variables, which is not the case for these here. In CMake, you usually have the pattern

FIND_LIBRARY(COREFOUNDATION_LIBRARY CoreFoundation) MARK_AS_ADVANCED(COREFOUNDATION_LIBRARY) SET(COREFOUNDATION_LIBRARIES ${COREFOUNDATION_LIBRARY})

So you have an intermediate variable here which is marked as advanced, not the final variable. Same applies to FindSecurity.

Yes, I was actually looking at some of CMake's modules for inspiration, and I suspect I misread LIBRARY/LIBRARIES. Will fix.

pks-t · 2017-09-22T08:06:04Z

+		FIND_PACKAGE(Security)
+		FIND_PACKAGE(CoreFoundation)
+	ENDIF()
+


Can't this even be moved into STREQUAL "SecureTransport"? Or is it required somewhere else?

I need to have checked for those so the auto-detection works (line 147 below). The other backends have their check before (OpenSSL is at line 125, line 103 is where we just trust WinHTTP is available).

tiennou · 2017-09-22T09:12:03Z

On the OpenSSL point, the CMake module itself looks borked: using -DOPENSSL_ROOT_DIR=/usr/local/opt/openssl@1.1, it will use headers from that location but link against stuff in /usr/lib.

I had been able to make it link to the correct libraries somehow with this branch, but I don't think it was possible before those changes (at least not without fixing up what the module fails to do, and obviously if you can fix, then you didn't really have a problem), because I don't think you could prevent the usage of SecureTransport.

It defaults to ON, e.g. "pick whatever default is appropriate for the platform". It accepts one of SecureTransport, OpenSSL, WinHTTP, or OFF. It errors if the backend library couldn't be found.

This allows us to only link against CoreFoundation when using the SecureTransport backend

tiennou · 2017-10-24T20:57:41Z

@pks-t This is rebased and ready for review !

pks-t · 2017-11-03T20:34:30Z

Thanks a lot! I'll review that next week.

pks-t · 2017-11-09T20:50:20Z

Sorry for taking that long with this PR. Thanks a lot for your patience!

tiennou · 2017-11-10T11:27:29Z

No sweat ! Imma happy contributor anyways 😉

tiennou · 2017-11-24T11:36:40Z

I found my old cmake incantation for building against OpenSSL on macOS, which I'll post here before it's purged from my shell history :

cmake .. -GXcode -DUSE_HTTPS=OpenSSL -DOPENSSL_ROOT_DIR=/usr/local/opt/openssl@1.1 -DOPENSSL_CRYPTO_LIBRARY=/usr/local/opt/openssl@1.1/lib/libcrypto.dylib -DOPENSSL_SSL_LIBRARY=/usr/local/opt/openssl@1.1/lib/libssl.dylib -DBUILD_SHARED_LIBS=ON

pks-t requested changes Jun 24, 2017

View reviewed changes

tiennou force-pushed the generic-tls branch from 10fdd8e to f955dd1 Compare July 2, 2017 14:24

pks-t requested changes Jul 3, 2017

View reviewed changes

tiennou commented Jul 7, 2017

View reviewed changes

tiennou force-pushed the generic-tls branch 2 times, most recently from 861ae4e to 4bd6da1 Compare August 1, 2017 22:36

tiennou force-pushed the generic-tls branch from 4bd6da1 to f8aa302 Compare August 9, 2017 22:04

tiennou force-pushed the generic-tls branch from f8aa302 to 8881e2a Compare August 10, 2017 18:03

tiennou force-pushed the generic-tls branch 2 times, most recently from 8117bd7 to 3d4cef5 Compare August 28, 2017 22:17

tiennou mentioned this pull request Aug 29, 2017

mbedTLS support #4173

Merged

2 tasks

pks-t requested changes Sep 1, 2017

View reviewed changes

tiennou force-pushed the generic-tls branch from 3d4cef5 to 496771e Compare September 6, 2017 22:43

pks-t requested changes Sep 22, 2017

View reviewed changes

tiennou added 3 commits October 23, 2017 20:02

cmake: simplify some HTTPS tests

08c1b8f

stream: Gather streams to src/streams

e936985

https: Prevent OpenSSL from namespace-leaking

2231705

tiennou added 8 commits October 23, 2017 20:02

clar: exit immediately on initialization failure

d3ef11e

https: correct some error messages

766b4dd

cmake: fix indentation before enhancing

152f376

cmake: make our macOS helpers more CMake-y

99d6ebb

cmake: use FeatureSummary to display which features we end up using

fdd0687

cmake: braces are not needed here

10b25db

cmake: Add USE_HTTPS as a CMake option

9980be0

It defaults to ON, e.g. "pick whatever default is appropriate for the platform". It accepts one of SecureTransport, OpenSSL, WinHTTP, or OFF. It errors if the backend library couldn't be found.

cmake: move Darwin-specific block around

c9bb68c

This allows us to only link against CoreFoundation when using the SecureTransport backend

tiennou force-pushed the generic-tls branch from 496771e to c9bb68c Compare October 23, 2017 18:04

tiennou mentioned this pull request Nov 5, 2017

Display dependency statuses (issue #3200) #4290

Closed

pks-t approved these changes Nov 7, 2017

View reviewed changes

pks-t merged commit 88450c1 into libgit2:master Nov 9, 2017

tiennou deleted the generic-tls branch November 10, 2017 11:27

carlosmn mentioned this pull request Jan 7, 2018

Have CMake show a summary of the build options/decisions #3200

Closed


		extern int git_openssl_stream_new(git_stream *out, const char host, const char *port);

		extern int git_openssl_set_cert_file(const char file, const char path);

		OPTION( DEBUG_POOL "Enable debug pool allocator" OFF )
		OPTION( ENABLE_WERROR "Enable compilation with -Werror" OFF )

Uh oh!

Conversation

tiennou commented Jun 23, 2017

Uh oh!

tiennou commented Jun 23, 2017

Uh oh!

pks-t commented Jun 24, 2017

Uh oh!

pks-t left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tiennou Jul 2, 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!

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!

pks-t commented Jun 24, 2017

Uh oh!

tiennou commented Jul 1, 2017

Uh oh!

pks-t commented Jul 3, 2017

Uh oh!

tiennou commented Jul 3, 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!

tiennou commented Jul 7, 2017

Uh oh!

Choose a reason for hiding this comment

tiennou Jul 2, 2017 •

edited

Loading