ARROW-10206: [C++][Python][FlightRPC] Allow disabling server validation #8325

jduo · 2020-10-02T08:08:03Z

@lidavidm WIP:

Demonstrate changes for C++ and Java clients.
Need to determine how to specify gRPC version in C++.
Need to check over coding style in C++ and Java code.
Python client changes pending.
Unit tests pending.
Does it make sense to deprecate use of SslCredentialsOptions in C++ and use TlsCredentialsOptions for other SSL properties? This is higher in blast radius and uses an experimental API for more, but would be more consistent.

github-actions · 2020-10-02T08:27:13Z

https://issues.apache.org/jira/browse/ARROW-10105

lidavidm · 2020-10-02T12:18:35Z

Hey James, I'll review this later today hopefully, but just to answer a few questions

Style in C++ can be done automatically with clang-format, see the developer guide.
For Python similarly you can use the flake8 config
The versions are scattered but as a start see thirdparty.txt: https://github.com/apache/arrow/blob/master/cpp/thirdparty/versions.txt though note you may also have to update CI scripts, Homebrew/Conda packages, etc.
I'd rather avoid very recent experimental features as that will create a lot of churn for us as gRPC upgrades frequently. It may be worth considering in a few versions if it looks like the API settles down, since I agree it would be nice to consolidate the options + it does bring the ability to do certificate rotation.

lidavidm · 2020-10-02T12:36:25Z

Actually, it seems the relevant changes have been there since early this year. It may be OK to switch fully.

lidavidm

Looks good to me as a start. Can unit tests be added? There are already tests set up to use (self-signed) certs that could serve as a starting point.

jduo · 2020-10-02T20:41:49Z

Hey James, I'll review this later today hopefully, but just to answer a few questions

Style in C++ can be done automatically with clang-format, see the developer guide.

For Python similarly you can use the flake8 config

The versions are scattered but as a start see thirdparty.txt: https://github.com/apache/arrow/blob/master/cpp/thirdparty/versions.txt though note you may also have to update CI scripts, Homebrew/Conda packages, etc.

I'd rather avoid very recent experimental features as that will create a lot of churn for us as gRPC upgrades frequently. It may be worth considering in a few versions if it looks like the API settles down, since I agree it would be nice to consolidate the options + it does bring the ability to do certificate rotation.

Hi @lidavidm .

I have made the Python edits now and am running them through CI while I get my environment working.
Unit tests have been added for all three languages.
I updated basically every file every file I could find that referenced a C++ gRPC version.

lidavidm · 2020-10-02T20:49:23Z

@kszucs - are we ok to bump the gRPC versions like this?

cpp/src/arrow/flight/client.h

Co-authored-by: Uwe L. Korn <[email protected]>

Also revert manylinux builds to use gRPC 1.29.1 instead of 1.32 as they were using previously.

Detect which namespace, if any that TlsCredentialsOptions is in, and conditionally compile the Flight client code to use that. If TlsCredentialsOptions is not available, or doesn't let you specify server verification options then disallow support for using disable server verification on the client.

wesm · 2020-10-08T22:14:04Z

There's some broken stuff for me locally with clang-8, I'm trying to fix

In file included from /home/wesm/code/arrow/cpp/src/arrow/flight/try_compile/check_tls_opts_127.cc:24:
In file included from /home/wesm/cpp-toolchain/include/grpcpp/grpcpp.h:58:
In file included from /home/wesm/cpp-toolchain/include/grpcpp/server.h:22:
In file included from /home/wesm/cpp-toolchain/include/grpcpp/server_impl.h:37:
In file included from /home/wesm/cpp-toolchain/include/grpcpp/impl/codegen/server_interface.h:31:
/home/wesm/cpp-toolchain/include/grpcpp/impl/codegen/rpc_service_method.h:51:16: error: parameter 'handler_data:' not found in the function declaration [-Werror,-Wdocumentation]
    /// \param handler_data: internal data for the handler.
               ^~~~~~~~~~~~~
/home/wesm/cpp-toolchain/include/grpcpp/impl/codegen/rpc_service_method.h:51:16: note: did you mean 'handler_data'?
    /// \param handler_data: internal data for the handler.
               ^~~~~~~~~~~~~
               handler_data
In file included from /home/wesm/code/arrow/cpp/src/arrow/flight/try_compile/check_tls_opts_127.cc:24:
In file included from /home/wesm/cpp-toolchain/include/grpcpp/grpcpp.h:58:
In file included from /home/wesm/cpp-toolchain/include/grpcpp/server.h:22:
In file included from /home/wesm/cpp-toolchain/include/grpcpp/server_impl.h:37:
In file included from /home/wesm/cpp-toolchain/include/grpcpp/impl/codegen/server_interface.h:32:
In file included from /home/wesm/cpp-toolchain/include/grpcpp/impl/codegen/server_context_impl.h:41:
/home/wesm/cpp-toolchain/include/grpcpp/impl/codegen/server_callback_impl.h:406:18: error: parameter 'ok' not found in the function declaration [-Werror,-Wdocumentation]
  /// \param[in] ok Was it successful? If false, no further write-side operation
                 ^~
/home/wesm/cpp-toolchain/include/grpcpp/impl/codegen/server_callback_impl.h:412:18: error: parameter 'ok' not found in the function declaration [-Werror,-Wdocumentation]
  /// \param[in] ok Was it successful? If false, no further read-side operation
                 ^~
/home/wesm/cpp-toolchain/include/grpcpp/impl/codegen/server_callback_impl.h:419:18: error: parameter 'ok' not found in the function declaration [-Werror,-Wdocumentation]
  /// \param[in] ok Was it successful? If false, no further write-side operation
                 ^~
In file included from /home/wesm/code/arrow/cpp/src/arrow/flight/try_compile/check_tls_opts_127.cc:24:
In file included from /home/wesm/cpp-toolchain/include/grpcpp/grpcpp.h:59:
/home/wesm/cpp-toolchain/include/grpcpp/server_builder.h:131:14: error: parameter 'selected_port[out]' not found in the function declaration [-Werror,-Wdocumentation]
  /// \param selected_port[out] If not `nullptr`, gets populated with the port
             ^~~~~~~~~~~~~~~~~~
/home/wesm/cpp-toolchain/include/grpcpp/server_builder.h:131:14: note: did you mean 'selected_port'?
  /// \param selected_port[out] If not `nullptr`, gets populated with the port
             ^~~~~~~~~~~~~~~~~~
             selected_port
/home/wesm/cpp-toolchain/include/grpcpp/server_builder.h:207:8: error: declaration is marked with '\deprecated' command but does not have a deprecation attribute [-Werror,-Wdocumentation-deprecated-sync]
  /// \deprecated For backward compatibility.
      ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/wesm/cpp-toolchain/include/grpcpp/server_builder.h:208:56: note: add a deprecation attribute to the declaration to silence this warning
  ServerBuilder& SetMaxMessageSize(int max_message_size) {
                                                       ^
                                                         __attribute__((deprecated))
/home/wesm/code/arrow/cpp/src/arrow/flight/try_compile/check_tls_opts_127.cc:28:11: error: no member named 'experimental' in namespace 'grpc_impl'; did you mean 'grpc::experimental'?
    const grpc_impl::experimental::TlsCredentialsOptions* options) {
          ^~~~~~~~~~~~~~~~~~~~~~~
          grpc::experimental
/home/wesm/cpp-toolchain/include/grpcpp/impl/codegen/server_context.h:34:11: note: 'grpc::experimental' declared here
namespace experimental {
          ^
/home/wesm/code/arrow/cpp/src/arrow/flight/try_compile/check_tls_opts_127.cc:34:39: error: unused variable 'opt' [-Werror,-Wunused-variable]
  grpc_tls_server_verification_option opt = check(nullptr);
                                      ^
8 errors generated.
ninja: build stopped: subcommand failed.

jduo · 2020-10-08T22:34:52Z

There's some broken stuff for me locally with clang-8, I'm trying to fix

In file included from /home/wesm/code/arrow/cpp/src/arrow/flight/try_compile/check_tls_opts_127.cc:24:
In file included from /home/wesm/cpp-toolchain/include/grpcpp/grpcpp.h:58:
In file included from /home/wesm/cpp-toolchain/include/grpcpp/server.h:22:
In file included from /home/wesm/cpp-toolchain/include/grpcpp/server_impl.h:37:
In file included from /home/wesm/cpp-toolchain/include/grpcpp/impl/codegen/server_interface.h:31:
/home/wesm/cpp-toolchain/include/grpcpp/impl/codegen/rpc_service_method.h:51:16: error: parameter 'handler_data:' not found in the function declaration [-Werror,-Wdocumentation]
    /// \param handler_data: internal data for the handler.
               ^~~~~~~~~~~~~
/home/wesm/cpp-toolchain/include/grpcpp/impl/codegen/rpc_service_method.h:51:16: note: did you mean 'handler_data'?
    /// \param handler_data: internal data for the handler.
               ^~~~~~~~~~~~~
               handler_data
In file included from /home/wesm/code/arrow/cpp/src/arrow/flight/try_compile/check_tls_opts_127.cc:24:
In file included from /home/wesm/cpp-toolchain/include/grpcpp/grpcpp.h:58:
In file included from /home/wesm/cpp-toolchain/include/grpcpp/server.h:22:
In file included from /home/wesm/cpp-toolchain/include/grpcpp/server_impl.h:37:
In file included from /home/wesm/cpp-toolchain/include/grpcpp/impl/codegen/server_interface.h:32:
In file included from /home/wesm/cpp-toolchain/include/grpcpp/impl/codegen/server_context_impl.h:41:
/home/wesm/cpp-toolchain/include/grpcpp/impl/codegen/server_callback_impl.h:406:18: error: parameter 'ok' not found in the function declaration [-Werror,-Wdocumentation]
  /// \param[in] ok Was it successful? If false, no further write-side operation
                 ^~
/home/wesm/cpp-toolchain/include/grpcpp/impl/codegen/server_callback_impl.h:412:18: error: parameter 'ok' not found in the function declaration [-Werror,-Wdocumentation]
  /// \param[in] ok Was it successful? If false, no further read-side operation
                 ^~
/home/wesm/cpp-toolchain/include/grpcpp/impl/codegen/server_callback_impl.h:419:18: error: parameter 'ok' not found in the function declaration [-Werror,-Wdocumentation]
  /// \param[in] ok Was it successful? If false, no further write-side operation
                 ^~
In file included from /home/wesm/code/arrow/cpp/src/arrow/flight/try_compile/check_tls_opts_127.cc:24:
In file included from /home/wesm/cpp-toolchain/include/grpcpp/grpcpp.h:59:
/home/wesm/cpp-toolchain/include/grpcpp/server_builder.h:131:14: error: parameter 'selected_port[out]' not found in the function declaration [-Werror,-Wdocumentation]
  /// \param selected_port[out] If not `nullptr`, gets populated with the port
             ^~~~~~~~~~~~~~~~~~
/home/wesm/cpp-toolchain/include/grpcpp/server_builder.h:131:14: note: did you mean 'selected_port'?
  /// \param selected_port[out] If not `nullptr`, gets populated with the port
             ^~~~~~~~~~~~~~~~~~
             selected_port
/home/wesm/cpp-toolchain/include/grpcpp/server_builder.h:207:8: error: declaration is marked with '\deprecated' command but does not have a deprecation attribute [-Werror,-Wdocumentation-deprecated-sync]
  /// \deprecated For backward compatibility.
      ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/wesm/cpp-toolchain/include/grpcpp/server_builder.h:208:56: note: add a deprecation attribute to the declaration to silence this warning
  ServerBuilder& SetMaxMessageSize(int max_message_size) {
                                                       ^
                                                         __attribute__((deprecated))
/home/wesm/code/arrow/cpp/src/arrow/flight/try_compile/check_tls_opts_127.cc:28:11: error: no member named 'experimental' in namespace 'grpc_impl'; did you mean 'grpc::experimental'?
    const grpc_impl::experimental::TlsCredentialsOptions* options) {
          ^~~~~~~~~~~~~~~~~~~~~~~
          grpc::experimental
/home/wesm/cpp-toolchain/include/grpcpp/impl/codegen/server_context.h:34:11: note: 'grpc::experimental' declared here
namespace experimental {
          ^
/home/wesm/code/arrow/cpp/src/arrow/flight/try_compile/check_tls_opts_127.cc:34:39: error: unused variable 'opt' [-Werror,-Wunused-variable]
  grpc_tls_server_verification_option opt = check(nullptr);
                                      ^
8 errors generated.
ninja: build stopped: subcommand failed.

The files in try_compile shouldn't be built as part of the project -- they should only be invoked by CMake for assessing grpc-cpp version information.

wesm · 2020-10-08T22:45:16Z

The files in try_compile shouldn't be built as part of the project -- they should only be invoked by CMake for assessing grpc-cpp version information.

Right, but the CMAKE_CXX_FLAGS set at that point are passed along, including -Werror. I'm pushing a fix here in a minute

jduo · 2020-10-08T23:01:01Z

The files in try_compile shouldn't be built as part of the project -- they should only be invoked by CMake for assessing grpc-cpp version information.

Right, but the CMAKE_CXX_FLAGS set at that point are passed along, including -Werror. I'm pushing a fix here in a minute

Thanks for fixing that Wes.

lidavidm

LGTM. I'll follow up on the ML.

emkornfield · 2020-10-09T02:54:30Z

cpp/src/arrow/flight/client.cc

+// requires root CA certs, even if you are skipping server
+// verification.
+#if defined(GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS)
+const char BLANK_ROOT_PEM[] =


nit: constexpr? or static.

Made a constexpr

emkornfield · 2020-10-09T02:58:21Z

cpp/src/arrow/flight/client.h


 class ARROW_FLIGHT_EXPORT FlightClientOptions {
 public:
+  FlightClientOptions();


there shouldn't be two ways of getting the default values. If there needs to be another constructor that takes credentials, then use the static factory pattern.

The default constructor was already there prior to this patch, it was just being implicitly defined instead of explicitly. I agree in principle that the Defaults() method should be used, however the constructor has already been public and I'm not sure it's worth breaking existing application code.

We're not really consistent about this internally either.
The C++ unit tests make use of both the public constructor and Defaults() method. The Python wrapper uses the public constructor.

Given there's already code using the default constructor, I think we can remove it separately: https://issues.apache.org/jira/browse/ARROW-10250

pitrou · 2020-10-09T10:35:09Z

cpp/src/arrow/flight/client.cc

+        if (options.disable_server_verification) {
+#if !defined(GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS)
+          return Status::NotImplemented(
+              "Using encryption with server verification is unsupported.");


Mention the reason in the error message?

pitrou · 2020-10-09T10:35:52Z

cpp/src/arrow/flight/client.cc

+              std::make_shared<NoOpTlsAuthorizationCheck>());
+          auto materials_config = std::make_shared<ge::TlsKeyMaterialsConfig>();
+          materials_config->set_pem_root_certs(BLANK_ROOT_PEM);
+          ge::TlsCredentialsOptions tls_options(


So gRPC has both TlsCredentialsOptions and SslCredentialOptions?

Yes, but they aren't interchangeable. One works against TlsCredentials and one works against SslCredentials. TlsCredentials is a newer, currently experimental API that can do everything SslCredentials can do and more, such as do certificate reloading and supplying custom callbacks for cert verification.

lidavidm · 2020-10-09T17:03:12Z

Waiting for CI here...

lidavidm

Ok, looks like the MacOS build is failing due to an unrelated reason (Cython not being found) and other PRs (and master) are failing in the same way. (Travis still pending...)

lidavidm · 2020-10-09T17:24:57Z

cpp/src/arrow/flight/client.h


 class ARROW_FLIGHT_EXPORT FlightClientOptions {
 public:
+  FlightClientOptions();


Given there's already code using the default constructor, I think we can remove it separately: https://issues.apache.org/jira/browse/ARROW-10250

wesm · 2020-10-09T17:33:56Z

thanks everyone

kou · 2020-10-09T21:53:56Z

cpp/src/arrow/flight/CMakeLists.txt

+# way to pass -isystem $GRPC_INCLUDE_DIR instead of -I$GRPC_INCLUDE_DIR
+set(CMAKE_CXX_FLAGS_BACKUP "${CMAKE_CXX_FLAGS}")
+string(REPLACE "/WX" "" CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS}")
+string(REPLACE "-Werror" "" CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS}")


This doesn't work with -Werror=SOMETHING: #8419

…efaults This is a small follow up from PR #8325 to encourage always using a Defaults() method to avoid accidentally leaving fields uninitialized. Closes #9728 from lidavidm/arrow-10250 Authored-by: David Li <[email protected]> Signed-off-by: Yibo Cai <[email protected]>

jduo force-pushed the ARROW-10105 branch from 18115ea to 5ee6307 Compare October 2, 2020 09:42

lidavidm reviewed Oct 2, 2020

View reviewed changes

jduo force-pushed the ARROW-10105 branch 3 times, most recently from 70029e3 to 3146745 Compare October 2, 2020 20:37

jduo force-pushed the ARROW-10105 branch from 3146745 to 77efa6a Compare October 2, 2020 20:47

jduo force-pushed the ARROW-10105 branch 9 times, most recently from 67ba576 to cff2212 Compare October 4, 2020 10:17

jduo marked this pull request as draft October 4, 2020 10:39

jduo force-pushed the ARROW-10105 branch 6 times, most recently from dd8664a to 981281f Compare October 4, 2020 12:56

lidavidm reviewed Oct 4, 2020

View reviewed changes

cpp/src/arrow/flight/client.h Outdated Show resolved Hide resolved

jduo force-pushed the ARROW-10105 branch 2 times, most recently from a516c3c to d054783 Compare October 4, 2020 20:18

jduo and others added 4 commits October 8, 2020 17:08

Update cpp/src/arrow/flight/client.cc

7c62e14

Co-authored-by: Uwe L. Korn <[email protected]>

Change gRPC version used in CI to 1.27 and higher

90dce00

Also revert manylinux builds to use gRPC 1.29.1 instead of 1.32 as they were using previously.

Formatting fixes for client.cc

976f7f8

Workaround for CMAKE_CXX_FLAGS propagation issue

e02647d

wesm force-pushed the ARROW-10105 branch from 9d646f8 to e02647d Compare October 8, 2020 22:47

lidavidm approved these changes Oct 9, 2020

View reviewed changes

emkornfield reviewed Oct 9, 2020

View reviewed changes

Use constexpr for the dummy certificate

136b03c

pitrou reviewed Oct 9, 2020

View reviewed changes

Clarified error message when feature is unsupported.

f1d44b9

jduo force-pushed the ARROW-10105 branch from 0be8aa1 to f1d44b9 Compare October 9, 2020 16:09

lidavidm approved these changes Oct 9, 2020

View reviewed changes

wesm closed this in 9e587be Oct 9, 2020

kou reviewed Oct 9, 2020

View reviewed changes

lidavidm mentioned this pull request Mar 16, 2021

ARROW-10250: [C++][FlightRPC] Consistently use FlightClientOptions::Defaults #9728

Closed

This was referenced Oct 10, 2020

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

Closed

[Python][C++][FlightRPC] Add client option to disable server validation #26210

Closed

ARROW-10206: [C++][Python][FlightRPC] Allow disabling server validation #8325

ARROW-10206: [C++][Python][FlightRPC] Allow disabling server validation #8325

Uh oh!

Conversation

jduo commented Oct 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 2, 2020

Uh oh!

lidavidm commented Oct 2, 2020

Uh oh!

lidavidm commented Oct 2, 2020

Uh oh!

lidavidm left a comment

Choose a reason for hiding this comment

Uh oh!

jduo commented Oct 2, 2020

Uh oh!

lidavidm commented Oct 2, 2020

Uh oh!

Uh oh!

wesm commented Oct 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jduo commented Oct 8, 2020

Uh oh!

wesm commented Oct 8, 2020

Uh oh!

jduo commented Oct 8, 2020

Uh oh!

lidavidm left a comment

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!

lidavidm commented Oct 9, 2020

Uh oh!

lidavidm 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!

wesm commented Oct 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

jduo commented Oct 2, 2020 •

edited

Loading

wesm commented Oct 8, 2020 •

edited

Loading

lidavidm left a comment •

edited

Loading