[FlightRPC] Add client option to disable certificate validation with TLS

[asfimport]

Users of Flight may want to disable certificate validation if they want to only use encryption. A use case might be that the Flight server uses a self-signed certificate and doesn't distribute a certificate for clients to use.

This feature would be to add an explicit option to FlightClient.Builder to disable certificate validation. Note that this should not happen implicitly if a client uses a TLS location, but does not set a certificate. The client should explicitly set this option so that they are fully aware that they are making a connection with reduced security.

Reporter: James Duong / @jduo
Assignee: James Duong / @jduo

Subtasks:

• [Java][FlightRPC] Add client option to disable server verification
• [Python][C++][FlightRPC] Add client option to disable server validation

PRs and other links:

• GitHub Pull Request #8325

_{Note: This issue was originally created as ARROW-10105. Please see the migration documentation for further details.}

[asfimport]

James Duong / @jduo:
Another problem I'm seeing is that this is appearing in existing C++ TLS tests after updating gRPC to 1.29 (this is with no other code changes), though the tests still succeed:
https://github.com/apache/arrow/pull/8325/checks?check_run_id=1205585340#step:8:3529
E1004 14:59:52.344787303 12247 ssl_security_connector.cc:263] Handshaker factory creation failed with TSI_INVALID_ARGUMENT.
E1004 14:59:52.344849303 12247 server_secure_chttp2.cc:81] {"created":"@1601823592.344842403","description":"Unable to create secure server with credentials of type Ssl","file":"../src/core/ext/transport/chttp2/server/secure/server_secure_chttp2.cc","file_line":63}

I don't think this was showing up before.

Existing python TLS tests do fail though:
https://github.com/apache/arrow/pull/8325/checks?check_run_id=1205585202#step:8:4378
E1004 14:40:34.743438426 7680 ssl_transport_security.cc:1439] Handshake failed with fatal error SSL_ERROR_SSL: error:1408F10B:SSL routines:ssl3_get_record:wrong version number.

I believe the test certificates are using SSL v3, which may be deprecated. I'm not sure if these two issues are related (eg, the server fails to properly start in C++ due to now SSL v3 deprecation).

[asfimport]

David Li / @lidavidm:
SSLv3 is simply what gRPC calls its implementation. The certificate is unrelated to the protocol being used, which is TLS. These messages are expected, some of the tests check for the fact that enabling TLS properly rejects self-signed certificates.

[asfimport]

David Li / @lidavidm:
In C++, I see the only test failing is the new test. In Python, "wrong version number" usually means a TLS-enabled client connected to a non-TLS server.

[asfimport]

David Li / @lidavidm:
I added a comment on the review for what I think the issue is.

[asfimport]

James Duong / @jduo:
Thanks @lidavidm, that seems to have helped with the failures in existing tests.

For the CentOS 5.11 build I will look into explicitly removing the gRPC included with the OS.

The newly-added tests for Python and C++ are failing. The TlsCredentials interface seems to require passing in a root PEM, whereas SslCredentials has will use a default if it's not supplied. It either uses a file based on an environment variable, or uses a CA cert supplied by gRPC's installation process. I don't see a way to access the latter though. It's in a non-exposed class:
https://github.com/grpc/grpc/blob/ff8ceb700e8a53ed4087edc006830da372b1199a/src/core/lib/security/security_connector/ssl_utils.cc#L525

I'll continue digging to see if there's a way to get to this path, but we may need to supply our own CA certs file. The content of the root certificate really shouldn't actually matter since this feature is to disable server verification. It would matter if we change TLS in general to use TlsCredentials.

[asfimport]

James Duong / @jduo:
Making some progress on this:

• I tested putting in a PEM for tls_roots_pem and see the new tests pass. I can confirm that it's running the callback I added to skip over cert verification – I was actually getting a crash due to this bug in gRPC until I wrote a workaround for it.

• The CentOS 5.11 build errors were from using gRPC 1.29 in build_grpc.sh scripts. This puts TlsCredentials and related classes in a different namespace than in 1.32 which is what brew and other environments used. grpc::experimental is used in 1.32 and grps_impl::experimental is used in older versions.

  I'm running into two build problems now:
  The first:
  Several Ursabot / AMD64 Conda builds are failing. I do not know where they are getting gRPC from or how they determine the gRPC version to get. eg: https://ci.ursalabs.org/#/builders/66/builds/11440

  The second:
  When I switch build_grpc.sh to compile gRPC 1.32, I get build errors building it on CentOS 5. The first was that it now has a new dependency on RE2, which I resolved. The second seems to be related to CentOS5 using an older Linux kernel (https://github.com/apache/arrow/pull/8325/checks?check_run_id=1208358439):

  
  CMakeFiles/grpc_unsecure.dir/src/core/lib/iomgr/socket_utils_common_posix.cc.o -c src/core/lib/iomgr/socket_utils_common_posix.cc
  In file included from /usr/include/asm-x86_64/byteorder.h:30:0,
                   from /usr/include/asm/byteorder.h:5,
                   from /usr/include/linux/tcp.h:21,
                   from src/core/lib/iomgr/socket_utils_common_posix.cc:34:
  /usr/include/linux/byteorder/little_endian.h:43:19: error: ‘__le64’ does not name a type
   static __inline__ __le64 __cpu_to_le64p(const __u64 *p)
                     ^
  /usr/include/linux/byteorder/little_endian.h:47:46: error: ‘__le64’ does not name a type
   static __inline__ __u64 __le64_to_cpup(const __le64 *p)
                                                ^
  /usr/include/linux/byteorder/little_endian.h:67:19: error: ‘__be64’ does not name a type
   static __inline__ __be64 __cpu_to_be64p(const __u64 *p)
                     ^
  /usr/include/linux/byteorder/little_endian.h:71:46: error: ‘__be64’ does not name a type
   static __inline__ __u64 __be64_to_cpup(const __be64 *p)
  

  I have read about a similar problem in QT where it could be resolved by using std=gnu+11 instead of -std=c+11. But we'd have to figure out a way to get gRPC's build system to do this. What's also odd is that particular class and the #include of linux/tcp.h have been around since gRPC 1.21. I don't think we could get away with just using an older gRPC either - we'll get build issues due to TlsCredentials being in different namespaces depending on gRPC versions. I don't see a way to define a macro to identify the namespace to look for TlsCredentials either – gRPC has a method to get the version at runtime but there wasn't an obvious way to get this through the preprocessor.

  There are also build failures in MinGW and R that seem to be issues downloading dependencies.

[asfimport]

James Duong / @jduo:
@lidavidm Update - I was able to get gRPC 1.32 to build on CentOSs 5. In 1.32, they essentially collapsed the logic that was for manylinux1 into the regular Linux case, but this seems to cause build failures on CentOS5 in practice.

I've reproduced the macro definitions that were used for manylinux1 prior to 1.32 and now gRPC compiles.
What fails now is when linking libarrow_flight.so:
ImportError: /arrow/python/pyarrow/libarrow_flight.so.200: undefined symbol: _ZN3re23RE2C1ERKSs
When demangled this is
ImportError: /arrow/python/pyarrow/libarrow_flight.so.200: undefined symbol: re2::RE2::RE2(std::string const&)

gRPC 1.32 added a dependency on RE2. I've added RE2 to Flight's CMakeLists, but that hasn't fixed this problem.
https://github.com/apache/arrow/blob/8ce02f7d5bd8d7cb732406af26bdc3b94333381b/cpp/src/arrow/flight/CMakeLists.txt#L23

I am also not understanding why the Ursa builds do not seem to get gRPC 1.32.

[asfimport]

David Li / @lidavidm:
@jduo I'm also not sure about the Ursabot builds, but from a short look, I think they may be using a fixed Conda environment instead of the one specified in the repository (e.g. the error message points to files that don't exist in the latest conda package). Notably see https://github.com/ursa-labs/ursabot/tree/master/projects/arrow and https://github.com/ursa-labs/ursabot/blob/05ec280304742f9795f30f589a60a5a1011d38cd/projects/arrow/docker/conda-cpp.txt

Maybe @xhochy or @kszucs could comment there.

I'm not sure about the CentOS build. Overall, I'd be hesitant merging something that changes the dependencies and build drastically right before the release cutoff unless the release manager is OK with it.

[asfimport]

James Duong / @jduo:
Thanks @lidavidm. I wasn't able to open the second link, but I'm assuming you're pointing to the grpc-cpp reference there that is using 1.21.:
https://github.com/ursa-labs/ursabot/blob/e958c5f95b31e98108df54cf13596c4fde944c3a/projects/arrow/docker/conda-cpp.txt#L19 ?

I don't know what the relationship of this repo is to the Arrow repo or how/when this gets updated. Be good if we could get some insight into this @xhochy and @kszucs.

I believe I've gotten past the RE2 linker error now on CentOS 5.11, but not sure if there are more CentOS 5 issues cropping up after. I have not implemented the change to use the dummy certificate. Related to this, I'm planning to just copy the root PEM file that ships with grpc that they put in /usr/share/grpc. Where would a good place be to put this in the source tree? And the install location would be /usr/share/arrow.

[asfimport]

James Duong / @jduo:
If we cannot change the environments now, would it make sense to include just the Java changes for Arrow 2.0 and add python/C++ to the January release?

[asfimport]

James Duong / @jduo:
I managed to get CentOS 5.11 / manylinux1 to build now.

It looks like the MinGW builds are failing because the latest gRPC release available is 1.29 on msys:
https://packages.msys2.org/package/mingw-w64-i686-grpc

1.29 does have the features needed, but they have them in the grpc_impl::experimental namespace rather than grpc::experimental namespace. We could make the namespace used a #define that's set differently for MinGW builds. (We also could have done this for CentOS5 but we'd basically have been just delaying dealing with upgrade challenges, so I felt the right thing to do was to resolve those now).

The URSA builds use 1.21.4 which doesn't have the TlsCredentials feature at all.

[asfimport]

David Li / @lidavidm:
If you would like to separate out the Java changes, those look straightforward and we can merge.

For the certs - seems gRPC wants the certs themselves and not necessarily a file, can we just embed the certs into the binary (or even, embed just a single invalid cert or something?)