You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/10/02 08:08:11 UTC
[GitHub] [arrow] jduo opened a new pull request #8325: ARROW-10105: [C++][Python][Java][FlightRPC] Allow disabling server validation
jduo opened a new pull request #8325:
URL: https://github.com/apache/arrow/pull/8325
WIP
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jduo commented on a change in pull request #8325: ARROW-10206: [C++][Python][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
jduo commented on a change in pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#discussion_r502009940
##########
File path: python/pyarrow/_flight.pyx
##########
@@ -1106,13 +1114,15 @@ cdef class FlightClient(_Weakrefable):
@classmethod
def connect(cls, location, tls_root_certs=None, cert_chain=None,
- private_key=None, override_hostname=None):
+ private_key=None, override_hostname=None,
+ disable_server_verify=None):
Review comment:
A few lines lower there's a line which is just disable_server_verification=disable_server_verification which exceeds the line limit. Do you know if there's a way around this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm commented on pull request #8325: ARROW-10206: [C++][Python][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
wesm commented on pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#issuecomment-705861730
> 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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jduo commented on pull request #8325: ARROW-10206: [C++][Python][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
jduo commented on pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#issuecomment-704794645
The Java work has been moved to #8377.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on a change in pull request #8325: ARROW-10206: [C++][Python][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#discussion_r501265265
##########
File path: cpp/cmake_modules/ThirdpartyToolchain.cmake
##########
@@ -2463,7 +2463,7 @@ macro(build_grpc)
endmacro()
if(ARROW_WITH_GRPC)
- set(ARROW_GRPC_REQUIRED_VERSION "1.17.0")
+ set(ARROW_GRPC_REQUIRED_VERSION "1.32.0")
Review comment:
@jduo on the mailing list, you say that it's 1.27 that is required
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm commented on pull request #8325: ARROW-10206: [C++][Python][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
wesm commented on pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#issuecomment-705851235
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jduo commented on a change in pull request #8325: ARROW-10206: [C++][Python][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
jduo commented on a change in pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#discussion_r501459974
##########
File path: python/manylinux1/scripts/build_grpc.sh
##########
@@ -16,8 +16,61 @@
# specific language governing permissions and limitations
# under the License.
-export GRPC_VERSION="1.29.1"
-export CFLAGS="-fPIC -DGPR_MANYLINUX1=1"
+export GRPC_VERSION="1.32.0"
Review comment:
Reverted this actually, since we only need gRPC 1.27 for this functionality to work.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #8325: ARROW-10206: [C++][Python][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#issuecomment-706296052
Waiting for CI here...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm commented on a change in pull request #8325: ARROW-10206: [C++][Python][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
wesm commented on a change in pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#discussion_r502040232
##########
File path: python/pyarrow/_flight.pyx
##########
@@ -1106,13 +1114,15 @@ cdef class FlightClient(_Weakrefable):
@classmethod
def connect(cls, location, tls_root_certs=None, cert_chain=None,
- private_key=None, override_hostname=None):
+ private_key=None, override_hostname=None,
+ disable_server_verify=None):
Review comment:
I'll push a change with alternative formatting that fixes this issue
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jduo commented on a change in pull request #8325: ARROW-10206: [C++][Python][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
jduo commented on a change in pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#discussion_r501459974
##########
File path: python/manylinux1/scripts/build_grpc.sh
##########
@@ -16,8 +16,61 @@
# specific language governing permissions and limitations
# under the License.
-export GRPC_VERSION="1.29.1"
-export CFLAGS="-fPIC -DGPR_MANYLINUX1=1"
+export GRPC_VERSION="1.32.0"
Review comment:
Reverted this actually, since we only need gRPC 1.27 for this functionality to work.
##########
File path: python/manylinux1/scripts/build_re2.sh
##########
@@ -31,5 +31,7 @@ make prefix=/usr/local -j${NCORES} install
popd
-# Need to remove shared library to make sure the static library is picked up by Arrow
-rm -rf re2-${RE2_VERSION}.tar.gz re2-${RE2_VERSION} /usr/local/lib/libre2.so*
+# Need to remove static library to make sure the shared library is picked up by Arrow
+# The static library fails to link when using gRPC 1.32.
+rm -rf re2-${RE2_VERSION}.tar.gz re2-${RE2_VERSION} /usr/local/lib/libre2.a
Review comment:
Reverted since we only need gRPC 1.27.
##########
File path: python/pyarrow/_flight.pyx
##########
@@ -1106,13 +1114,15 @@ cdef class FlightClient(_Weakrefable):
@classmethod
def connect(cls, location, tls_root_certs=None, cert_chain=None,
- private_key=None, override_hostname=None):
+ private_key=None, override_hostname=None,
+ disable_server_verify=None):
Review comment:
A few lines lower there's a line which is just disable_server_verification=disable_server_verification which exceeds the line limit. Do you know if there's a way around this?
##########
File path: cpp/src/arrow/flight/client.cc
##########
@@ -845,18 +878,52 @@ class FlightClient::FlightClientImpl {
if (scheme == kSchemeGrpc || scheme == kSchemeGrpcTcp || scheme == kSchemeGrpcTls) {
grpc_uri << location.uri_->host() << ":" << location.uri_->port_text();
- if (scheme == "grpc+tls") {
- grpc::SslCredentialsOptions ssl_options;
- if (!options.tls_root_certs.empty()) {
- ssl_options.pem_root_certs = options.tls_root_certs;
- }
- if (!options.cert_chain.empty()) {
- ssl_options.pem_cert_chain = options.cert_chain;
- }
- if (!options.private_key.empty()) {
- ssl_options.pem_private_key = options.private_key;
+ if (scheme == kSchemeGrpcTls) {
+ if (options.disable_server_verification) {
+#if !defined(GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS)
+ return Status::NotImplemented(
+ "Using encryption with server verification is unsupported.");
+#else
+ namespace ge = GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS;
+
+ // A callback to supply to TlsCredentialsOptions that accepts any server
+ // arguments.
+ struct NoOpTlsAuthorizationCheck
+ : public ge::TlsServerAuthorizationCheckInterface {
+ int Schedule(ge::TlsServerAuthorizationCheckArg* arg) override {
+ arg->set_success(1);
+ arg->set_status(GRPC_STATUS_OK);
+ return 0;
+ }
+ };
+
+ noOpAuthCheck = std::shared_ptr<ge::TlsServerAuthorizationCheckConfig>(
Review comment:
Done.
##########
File path: cpp/src/arrow/flight/client.cc
##########
@@ -845,18 +878,52 @@ class FlightClient::FlightClientImpl {
if (scheme == kSchemeGrpc || scheme == kSchemeGrpcTcp || scheme == kSchemeGrpcTls) {
grpc_uri << location.uri_->host() << ":" << location.uri_->port_text();
- if (scheme == "grpc+tls") {
- grpc::SslCredentialsOptions ssl_options;
- if (!options.tls_root_certs.empty()) {
- ssl_options.pem_root_certs = options.tls_root_certs;
- }
- if (!options.cert_chain.empty()) {
- ssl_options.pem_cert_chain = options.cert_chain;
- }
- if (!options.private_key.empty()) {
- ssl_options.pem_private_key = options.private_key;
+ if (scheme == kSchemeGrpcTls) {
+ if (options.disable_server_verification) {
+#if !defined(GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS)
+ return Status::NotImplemented(
+ "Using encryption with server verification is unsupported.");
+#else
+ namespace ge = GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS;
+
+ // A callback to supply to TlsCredentialsOptions that accepts any server
+ // arguments.
+ struct NoOpTlsAuthorizationCheck
+ : public ge::TlsServerAuthorizationCheckInterface {
+ int Schedule(ge::TlsServerAuthorizationCheckArg* arg) override {
+ arg->set_success(1);
+ arg->set_status(GRPC_STATUS_OK);
+ return 0;
+ }
+ };
+
+ noOpAuthCheck = std::shared_ptr<ge::TlsServerAuthorizationCheckConfig>(
+ new ge::TlsServerAuthorizationCheckConfig(
+ std::shared_ptr<ge::TlsServerAuthorizationCheckInterface>(
+ new NoOpTlsAuthorizationCheck())));
Review comment:
Done
##########
File path: cpp/src/arrow/flight/client.cc
##########
@@ -845,18 +878,52 @@ class FlightClient::FlightClientImpl {
if (scheme == kSchemeGrpc || scheme == kSchemeGrpcTcp || scheme == kSchemeGrpcTls) {
grpc_uri << location.uri_->host() << ":" << location.uri_->port_text();
- if (scheme == "grpc+tls") {
- grpc::SslCredentialsOptions ssl_options;
- if (!options.tls_root_certs.empty()) {
- ssl_options.pem_root_certs = options.tls_root_certs;
- }
- if (!options.cert_chain.empty()) {
- ssl_options.pem_cert_chain = options.cert_chain;
- }
- if (!options.private_key.empty()) {
- ssl_options.pem_private_key = options.private_key;
+ if (scheme == kSchemeGrpcTls) {
+ if (options.disable_server_verification) {
+#if !defined(GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS)
+ return Status::NotImplemented(
+ "Using encryption with server verification is unsupported.");
+#else
+ namespace ge = GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS;
+
+ // A callback to supply to TlsCredentialsOptions that accepts any server
+ // arguments.
+ struct NoOpTlsAuthorizationCheck
+ : public ge::TlsServerAuthorizationCheckInterface {
+ int Schedule(ge::TlsServerAuthorizationCheckArg* arg) override {
+ arg->set_success(1);
+ arg->set_status(GRPC_STATUS_OK);
+ return 0;
+ }
+ };
+
+ noOpAuthCheck = std::shared_ptr<ge::TlsServerAuthorizationCheckConfig>(
+ new ge::TlsServerAuthorizationCheckConfig(
+ std::shared_ptr<ge::TlsServerAuthorizationCheckInterface>(
+ new NoOpTlsAuthorizationCheck())));
+ std::shared_ptr<ge::TlsKeyMaterialsConfig> materials_config(
+ new ge::TlsKeyMaterialsConfig());
Review comment:
Done.
##########
File path: cpp/src/arrow/flight/client.h
##########
@@ -90,6 +90,8 @@ class ARROW_FLIGHT_EXPORT FlightWriteSizeStatusDetail : public arrow::StatusDeta
class ARROW_FLIGHT_EXPORT FlightClientOptions {
public:
+ FlightClientOptions();
Review comment:
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.
##########
File path: cpp/src/arrow/flight/client.cc
##########
@@ -835,6 +843,31 @@ class GrpcMetadataReader : public FlightMetadataReader {
std::shared_ptr<std::mutex> read_mutex_;
};
+namespace {
+// Dummy self-signed certificate to be used because TlsCredentials
+// requires root CA certs, even if you are skipping server
+// verification.
+#if defined(GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS)
+const char BLANK_ROOT_PEM[] =
Review comment:
Made a constexpr
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a change in pull request #8325: ARROW-10105: [C++][Python][Java][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
lidavidm commented on a change in pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#discussion_r499664483
##########
File path: cpp/cmake_modules/ThirdpartyToolchain.cmake
##########
@@ -2463,7 +2463,7 @@ macro(build_grpc)
endmacro()
if(ARROW_WITH_GRPC)
- set(ARROW_GRPC_REQUIRED_VERSION "1.17.0")
+ set(ARROW_GRPC_REQUIRED_VERSION "1.32.0")
Review comment:
Both seem fine, fortunately.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a change in pull request #8325: ARROW-10206: [C++][Python][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
lidavidm commented on a change in pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#discussion_r502572751
##########
File path: cpp/src/arrow/flight/client.h
##########
@@ -90,6 +90,8 @@ class ARROW_FLIGHT_EXPORT FlightWriteSizeStatusDetail : public arrow::StatusDeta
class ARROW_FLIGHT_EXPORT FlightClientOptions {
public:
+ FlightClientOptions();
Review comment:
Given there's already code using the default constructor, I think we can remove it separately: https://issues.apache.org/jira/browse/ARROW-10250
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jduo commented on a change in pull request #8325: ARROW-10206: [C++][Python][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
jduo commented on a change in pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#discussion_r502241411
##########
File path: cpp/src/arrow/flight/client.h
##########
@@ -90,6 +90,8 @@ class ARROW_FLIGHT_EXPORT FlightWriteSizeStatusDetail : public arrow::StatusDeta
class ARROW_FLIGHT_EXPORT FlightClientOptions {
public:
+ FlightClientOptions();
Review comment:
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #8325: ARROW-10105: [C++][Python][Java][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#issuecomment-702702219
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](https://arrow.apache.org/docs/developers/cpp/development.html).
- For Python similarly you can use the [flake8 config](https://arrow.apache.org/docs/developers/python.html#coding-style)
- 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] xhochy commented on a change in pull request #8325: ARROW-10206: [C++][Python][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
xhochy commented on a change in pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#discussion_r500933507
##########
File path: cpp/src/arrow/flight/client.cc
##########
@@ -34,6 +34,9 @@
#include <grpc++/grpc++.h>
#endif
+#include "grpc/grpc_security_constants.h"
+#include "grpcpp/security/tls_credentials_options.h"
Review comment:
External includes, thus no `"`
```suggestion
#include <grpc/grpc_security_constants.h>
#include <grpcpp/security/tls_credentials_options.h>
```
##########
File path: python/manylinux1/scripts/build_re2.sh
##########
@@ -31,5 +31,7 @@ make prefix=/usr/local -j${NCORES} install
popd
-# Need to remove shared library to make sure the static library is picked up by Arrow
-rm -rf re2-${RE2_VERSION}.tar.gz re2-${RE2_VERSION} /usr/local/lib/libre2.so*
+# Need to remove static library to make sure the shared library is picked up by Arrow
+# The static library fails to link when using gRPC 1.32.
+rm -rf re2-${RE2_VERSION}.tar.gz re2-${RE2_VERSION} /usr/local/lib/libre2.a
Review comment:
Why was this changed? Manylinux builds should be all static linkage.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jduo commented on a change in pull request #8325: ARROW-10206: [C++][Python][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
jduo commented on a change in pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#discussion_r501274245
##########
File path: cpp/cmake_modules/ThirdpartyToolchain.cmake
##########
@@ -2463,7 +2463,7 @@ macro(build_grpc)
endmacro()
if(ARROW_WITH_GRPC)
- set(ARROW_GRPC_REQUIRED_VERSION "1.17.0")
+ set(ARROW_GRPC_REQUIRED_VERSION "1.32.0")
Review comment:
Yeah that's correct. The feature is in 1.27, but in a different namespace. When I wrote this I figured for convenience of dev we'd want to use the latest stable release but now see that 1.32 isn't actually available in all our supported environments (MSYS only has 1.29). So instead what I'm planning:
- Define a macro indicating if TlsCredentialsOptions is supported. I'll see if it's possible to auto-detect this.
- Define a macro for the namespace holding TlsCredentialsOptions that is based on version.
- Compile out use of TlsCredentials based on the first macro.
- Revert changes in this PR that update the version the gRPC version 1.32.
- Change the minimum version in the codebase/CI scripts to 1.27 (only if it is already lower).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jduo commented on pull request #8325: ARROW-10206: [C++][Python][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
jduo commented on pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#issuecomment-705866472
> > 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] emkornfield commented on a change in pull request #8325: ARROW-10206: [C++][Python][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#discussion_r502156886
##########
File path: cpp/src/arrow/flight/client.cc
##########
@@ -835,6 +843,31 @@ class GrpcMetadataReader : public FlightMetadataReader {
std::shared_ptr<std::mutex> read_mutex_;
};
+namespace {
+// Dummy self-signed certificate to be used because TlsCredentials
+// requires root CA certs, even if you are skipping server
+// verification.
+#if defined(GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS)
+const char BLANK_ROOT_PEM[] =
Review comment:
nit: constexpr? or static.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a change in pull request #8325: ARROW-10105: [C++][Python][Java][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
lidavidm commented on a change in pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#discussion_r499282185
##########
File path: cpp/src/arrow/flight/client.h
##########
@@ -108,10 +108,14 @@ class ARROW_FLIGHT_EXPORT FlightClientOptions {
/// positive. When enabled, FlightStreamWriter.Write* may yield a
/// IOError with error detail FlightWriteSizeStatusDetail.
int64_t write_size_limit_bytes;
+
/// \brief Generic connection options, passed to the underlying
/// transport; interpretation is implementation-dependent.
std::vector<std::pair<std::string, util::variant<int, std::string>>> generic_options;
+ /// \brief Use TLS without validating the server certificate. Use with caution.
+ bool disable_server_verification;
+
/// \brief Get default options.
static FlightClientOptions Defaults();
Review comment:
I believe you're seeing failures since `Defaults` should be updated to set the new flag, else the flag may or may not be initialized.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jduo commented on a change in pull request #8325: ARROW-10105: [C++][Python][Java][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
jduo commented on a change in pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#discussion_r499306998
##########
File path: cpp/src/arrow/flight/client.h
##########
@@ -108,10 +108,14 @@ class ARROW_FLIGHT_EXPORT FlightClientOptions {
/// positive. When enabled, FlightStreamWriter.Write* may yield a
/// IOError with error detail FlightWriteSizeStatusDetail.
int64_t write_size_limit_bytes;
+
/// \brief Generic connection options, passed to the underlying
/// transport; interpretation is implementation-dependent.
std::vector<std::pair<std::string, util::variant<int, std::string>>> generic_options;
+ /// \brief Use TLS without validating the server certificate. Use with caution.
+ bool disable_server_verification;
+
/// \brief Get default options.
static FlightClientOptions Defaults();
Review comment:
I've added an explicitly defined constructor since users might be constructing FlightCredentialsOptions directly. I am surprised that this didn't previously cause problems with the write_size_limit_bytes option though.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #8325: ARROW-10105: [C++][Python][Java][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#issuecomment-702951063
@kszucs - are we ok to bump the gRPC versions like this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #8325: ARROW-10206: [C++][Python][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#discussion_r502339168
##########
File path: cpp/src/arrow/flight/client.cc
##########
@@ -845,18 +878,48 @@ class FlightClient::FlightClientImpl {
if (scheme == kSchemeGrpc || scheme == kSchemeGrpcTcp || scheme == kSchemeGrpcTls) {
grpc_uri << location.uri_->host() << ":" << location.uri_->port_text();
- if (scheme == "grpc+tls") {
- grpc::SslCredentialsOptions ssl_options;
- if (!options.tls_root_certs.empty()) {
- ssl_options.pem_root_certs = options.tls_root_certs;
- }
- if (!options.cert_chain.empty()) {
- ssl_options.pem_cert_chain = options.cert_chain;
- }
- if (!options.private_key.empty()) {
- ssl_options.pem_private_key = options.private_key;
+ if (scheme == kSchemeGrpcTls) {
+ if (options.disable_server_verification) {
+#if !defined(GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS)
+ return Status::NotImplemented(
+ "Using encryption with server verification is unsupported.");
+#else
+ namespace ge = GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS;
+
+ // A callback to supply to TlsCredentialsOptions that accepts any server
+ // arguments.
+ struct NoOpTlsAuthorizationCheck
+ : public ge::TlsServerAuthorizationCheckInterface {
+ int Schedule(ge::TlsServerAuthorizationCheckArg* arg) override {
+ arg->set_success(1);
+ arg->set_status(GRPC_STATUS_OK);
+ return 0;
+ }
+ };
+
+ noop_auth_check_ = std::make_shared<ge::TlsServerAuthorizationCheckConfig>(
+ 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(
Review comment:
So gRPC has both `TlsCredentialsOptions` and `SslCredentialOptions`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on a change in pull request #8325: ARROW-10206: [C++][Python][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
kou commented on a change in pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#discussion_r502686050
##########
File path: cpp/src/arrow/flight/CMakeLists.txt
##########
@@ -61,6 +61,48 @@ set_source_files_properties(${FLIGHT_GENERATED_PROTO_FILES} PROPERTIES GENERATED
add_custom_target(flight_grpc_gen ALL DEPENDS ${FLIGHT_GENERATED_PROTO_FILES})
+# <KLUDGE> -Werror / /WX cause try_compile to fail because there seems to be no
+# 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}")
Review comment:
This doesn't work with `-Werror=SOMETHING`: #8419
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] emkornfield commented on a change in pull request #8325: ARROW-10206: [C++][Python][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#discussion_r502157792
##########
File path: cpp/src/arrow/flight/client.h
##########
@@ -90,6 +90,8 @@ class ARROW_FLIGHT_EXPORT FlightWriteSizeStatusDetail : public arrow::StatusDeta
class ARROW_FLIGHT_EXPORT FlightClientOptions {
public:
+ FlightClientOptions();
Review comment:
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jduo commented on pull request #8325: ARROW-10206: [C++][Python][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
jduo commented on pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#issuecomment-705858448
> 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jduo commented on pull request #8325: ARROW-10105: [C++][Python][Java][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
jduo commented on pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#issuecomment-702948147
> 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](https://arrow.apache.org/docs/developers/cpp/development.html).
> * For Python similarly you can use the [flake8 config](https://arrow.apache.org/docs/developers/python.html#coding-style)
> * 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm commented on a change in pull request #8325: ARROW-10206: [C++][Python][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
wesm commented on a change in pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#discussion_r502002828
##########
File path: cpp/src/arrow/flight/client.cc
##########
@@ -845,18 +878,52 @@ class FlightClient::FlightClientImpl {
if (scheme == kSchemeGrpc || scheme == kSchemeGrpcTcp || scheme == kSchemeGrpcTls) {
grpc_uri << location.uri_->host() << ":" << location.uri_->port_text();
- if (scheme == "grpc+tls") {
- grpc::SslCredentialsOptions ssl_options;
- if (!options.tls_root_certs.empty()) {
- ssl_options.pem_root_certs = options.tls_root_certs;
- }
- if (!options.cert_chain.empty()) {
- ssl_options.pem_cert_chain = options.cert_chain;
- }
- if (!options.private_key.empty()) {
- ssl_options.pem_private_key = options.private_key;
+ if (scheme == kSchemeGrpcTls) {
+ if (options.disable_server_verification) {
+#if !defined(GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS)
+ return Status::NotImplemented(
+ "Using encryption with server verification is unsupported.");
+#else
+ namespace ge = GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS;
+
+ // A callback to supply to TlsCredentialsOptions that accepts any server
+ // arguments.
+ struct NoOpTlsAuthorizationCheck
+ : public ge::TlsServerAuthorizationCheckInterface {
+ int Schedule(ge::TlsServerAuthorizationCheckArg* arg) override {
+ arg->set_success(1);
+ arg->set_status(GRPC_STATUS_OK);
+ return 0;
+ }
+ };
+
+ noOpAuthCheck = std::shared_ptr<ge::TlsServerAuthorizationCheckConfig>(
+ new ge::TlsServerAuthorizationCheckConfig(
Review comment:
Can you use `make_shared` here?
##########
File path: cpp/src/arrow/flight/client.cc
##########
@@ -845,18 +878,52 @@ class FlightClient::FlightClientImpl {
if (scheme == kSchemeGrpc || scheme == kSchemeGrpcTcp || scheme == kSchemeGrpcTls) {
grpc_uri << location.uri_->host() << ":" << location.uri_->port_text();
- if (scheme == "grpc+tls") {
- grpc::SslCredentialsOptions ssl_options;
- if (!options.tls_root_certs.empty()) {
- ssl_options.pem_root_certs = options.tls_root_certs;
- }
- if (!options.cert_chain.empty()) {
- ssl_options.pem_cert_chain = options.cert_chain;
- }
- if (!options.private_key.empty()) {
- ssl_options.pem_private_key = options.private_key;
+ if (scheme == kSchemeGrpcTls) {
+ if (options.disable_server_verification) {
+#if !defined(GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS)
+ return Status::NotImplemented(
+ "Using encryption with server verification is unsupported.");
+#else
+ namespace ge = GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS;
+
+ // A callback to supply to TlsCredentialsOptions that accepts any server
+ // arguments.
+ struct NoOpTlsAuthorizationCheck
+ : public ge::TlsServerAuthorizationCheckInterface {
+ int Schedule(ge::TlsServerAuthorizationCheckArg* arg) override {
+ arg->set_success(1);
+ arg->set_status(GRPC_STATUS_OK);
+ return 0;
+ }
+ };
+
+ noOpAuthCheck = std::shared_ptr<ge::TlsServerAuthorizationCheckConfig>(
Review comment:
Use `no_op_auth_check_`
##########
File path: cpp/src/arrow/flight/client.cc
##########
@@ -845,18 +878,52 @@ class FlightClient::FlightClientImpl {
if (scheme == kSchemeGrpc || scheme == kSchemeGrpcTcp || scheme == kSchemeGrpcTls) {
grpc_uri << location.uri_->host() << ":" << location.uri_->port_text();
- if (scheme == "grpc+tls") {
- grpc::SslCredentialsOptions ssl_options;
- if (!options.tls_root_certs.empty()) {
- ssl_options.pem_root_certs = options.tls_root_certs;
- }
- if (!options.cert_chain.empty()) {
- ssl_options.pem_cert_chain = options.cert_chain;
- }
- if (!options.private_key.empty()) {
- ssl_options.pem_private_key = options.private_key;
+ if (scheme == kSchemeGrpcTls) {
+ if (options.disable_server_verification) {
+#if !defined(GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS)
+ return Status::NotImplemented(
+ "Using encryption with server verification is unsupported.");
+#else
+ namespace ge = GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS;
+
+ // A callback to supply to TlsCredentialsOptions that accepts any server
+ // arguments.
+ struct NoOpTlsAuthorizationCheck
+ : public ge::TlsServerAuthorizationCheckInterface {
+ int Schedule(ge::TlsServerAuthorizationCheckArg* arg) override {
+ arg->set_success(1);
+ arg->set_status(GRPC_STATUS_OK);
+ return 0;
+ }
+ };
+
+ noOpAuthCheck = std::shared_ptr<ge::TlsServerAuthorizationCheckConfig>(
+ new ge::TlsServerAuthorizationCheckConfig(
+ std::shared_ptr<ge::TlsServerAuthorizationCheckInterface>(
+ new NoOpTlsAuthorizationCheck())));
+ std::shared_ptr<ge::TlsKeyMaterialsConfig> materials_config(
+ new ge::TlsKeyMaterialsConfig());
Review comment:
`auto materials_config = std::make_shared<ge::TlsKeyMaterialsConfig>();`?
##########
File path: cpp/src/arrow/flight/client.cc
##########
@@ -845,18 +878,52 @@ class FlightClient::FlightClientImpl {
if (scheme == kSchemeGrpc || scheme == kSchemeGrpcTcp || scheme == kSchemeGrpcTls) {
grpc_uri << location.uri_->host() << ":" << location.uri_->port_text();
- if (scheme == "grpc+tls") {
- grpc::SslCredentialsOptions ssl_options;
- if (!options.tls_root_certs.empty()) {
- ssl_options.pem_root_certs = options.tls_root_certs;
- }
- if (!options.cert_chain.empty()) {
- ssl_options.pem_cert_chain = options.cert_chain;
- }
- if (!options.private_key.empty()) {
- ssl_options.pem_private_key = options.private_key;
+ if (scheme == kSchemeGrpcTls) {
+ if (options.disable_server_verification) {
+#if !defined(GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS)
+ return Status::NotImplemented(
+ "Using encryption with server verification is unsupported.");
+#else
+ namespace ge = GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS;
+
+ // A callback to supply to TlsCredentialsOptions that accepts any server
+ // arguments.
+ struct NoOpTlsAuthorizationCheck
+ : public ge::TlsServerAuthorizationCheckInterface {
+ int Schedule(ge::TlsServerAuthorizationCheckArg* arg) override {
+ arg->set_success(1);
+ arg->set_status(GRPC_STATUS_OK);
+ return 0;
+ }
+ };
+
+ noOpAuthCheck = std::shared_ptr<ge::TlsServerAuthorizationCheckConfig>(
+ new ge::TlsServerAuthorizationCheckConfig(
+ std::shared_ptr<ge::TlsServerAuthorizationCheckInterface>(
+ new NoOpTlsAuthorizationCheck())));
Review comment:
possible to use `make_shared`?
##########
File path: python/pyarrow/_flight.pyx
##########
@@ -1106,13 +1114,15 @@ cdef class FlightClient(_Weakrefable):
@classmethod
def connect(cls, location, tls_root_certs=None, cert_chain=None,
- private_key=None, override_hostname=None):
+ private_key=None, override_hostname=None,
+ disable_server_verify=None):
Review comment:
Make this consistent? verify -> verification
##########
File path: cpp/src/arrow/flight/client.cc
##########
@@ -845,18 +878,52 @@ class FlightClient::FlightClientImpl {
if (scheme == kSchemeGrpc || scheme == kSchemeGrpcTcp || scheme == kSchemeGrpcTls) {
grpc_uri << location.uri_->host() << ":" << location.uri_->port_text();
- if (scheme == "grpc+tls") {
- grpc::SslCredentialsOptions ssl_options;
- if (!options.tls_root_certs.empty()) {
- ssl_options.pem_root_certs = options.tls_root_certs;
- }
- if (!options.cert_chain.empty()) {
- ssl_options.pem_cert_chain = options.cert_chain;
- }
- if (!options.private_key.empty()) {
- ssl_options.pem_private_key = options.private_key;
+ if (scheme == kSchemeGrpcTls) {
+ if (options.disable_server_verification) {
+#if !defined(GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS)
+ return Status::NotImplemented(
+ "Using encryption with server verification is unsupported.");
+#else
+ namespace ge = GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS;
+
+ // A callback to supply to TlsCredentialsOptions that accepts any server
+ // arguments.
+ struct NoOpTlsAuthorizationCheck
+ : public ge::TlsServerAuthorizationCheckInterface {
+ int Schedule(ge::TlsServerAuthorizationCheckArg* arg) override {
+ arg->set_success(1);
+ arg->set_status(GRPC_STATUS_OK);
+ return 0;
+ }
+ };
+
+ noOpAuthCheck = std::shared_ptr<ge::TlsServerAuthorizationCheckConfig>(
Review comment:
Use `no_op_auth_check_` (or `noop_auth_check_`, whichever is more consistent with how we write it elsewhere in the codebase)
##########
File path: python/pyarrow/_flight.pyx
##########
@@ -1106,13 +1114,15 @@ cdef class FlightClient(_Weakrefable):
@classmethod
def connect(cls, location, tls_root_certs=None, cert_chain=None,
- private_key=None, override_hostname=None):
+ private_key=None, override_hostname=None,
+ disable_server_verify=None):
Review comment:
I'll push a change with alternative formatting that fixes this issue
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #8325: ARROW-10105: [C++][Python][Java][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#issuecomment-702597746
https://issues.apache.org/jira/browse/ARROW-10105
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm closed pull request #8325: ARROW-10206: [C++][Python][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
wesm closed pull request #8325:
URL: https://github.com/apache/arrow/pull/8325
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a change in pull request #8325: ARROW-10105: [C++][Python][Java][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
lidavidm commented on a change in pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#discussion_r499662614
##########
File path: cpp/cmake_modules/ThirdpartyToolchain.cmake
##########
@@ -2463,7 +2463,7 @@ macro(build_grpc)
endmacro()
if(ARROW_WITH_GRPC)
- set(ARROW_GRPC_REQUIRED_VERSION "1.17.0")
+ set(ARROW_GRPC_REQUIRED_VERSION "1.32.0")
Review comment:
We should check that Homebrew and Conda, at least, have this packaged.
##########
File path: dev/tasks/conda-recipes/.ci_support/linux_aarch64_python3.6.____cpython.yaml
##########
@@ -31,7 +31,7 @@ gflags:
glog:
- 0.4.0
grpc_cpp:
-- '1.30'
+- '1.32.0'
Review comment:
Can we avoid pinning a minor version?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #8325: ARROW-10206: [C++][Python][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#discussion_r502338793
##########
File path: cpp/src/arrow/flight/client.cc
##########
@@ -845,18 +878,48 @@ class FlightClient::FlightClientImpl {
if (scheme == kSchemeGrpc || scheme == kSchemeGrpcTcp || scheme == kSchemeGrpcTls) {
grpc_uri << location.uri_->host() << ":" << location.uri_->port_text();
- if (scheme == "grpc+tls") {
- grpc::SslCredentialsOptions ssl_options;
- if (!options.tls_root_certs.empty()) {
- ssl_options.pem_root_certs = options.tls_root_certs;
- }
- if (!options.cert_chain.empty()) {
- ssl_options.pem_cert_chain = options.cert_chain;
- }
- if (!options.private_key.empty()) {
- ssl_options.pem_private_key = options.private_key;
+ if (scheme == kSchemeGrpcTls) {
+ if (options.disable_server_verification) {
+#if !defined(GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS)
+ return Status::NotImplemented(
+ "Using encryption with server verification is unsupported.");
Review comment:
Mention the reason in the error message?
##########
File path: cpp/src/arrow/flight/client.cc
##########
@@ -845,18 +878,48 @@ class FlightClient::FlightClientImpl {
if (scheme == kSchemeGrpc || scheme == kSchemeGrpcTcp || scheme == kSchemeGrpcTls) {
grpc_uri << location.uri_->host() << ":" << location.uri_->port_text();
- if (scheme == "grpc+tls") {
- grpc::SslCredentialsOptions ssl_options;
- if (!options.tls_root_certs.empty()) {
- ssl_options.pem_root_certs = options.tls_root_certs;
- }
- if (!options.cert_chain.empty()) {
- ssl_options.pem_cert_chain = options.cert_chain;
- }
- if (!options.private_key.empty()) {
- ssl_options.pem_private_key = options.private_key;
+ if (scheme == kSchemeGrpcTls) {
+ if (options.disable_server_verification) {
+#if !defined(GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS)
+ return Status::NotImplemented(
+ "Using encryption with server verification is unsupported.");
+#else
+ namespace ge = GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS;
+
+ // A callback to supply to TlsCredentialsOptions that accepts any server
+ // arguments.
+ struct NoOpTlsAuthorizationCheck
+ : public ge::TlsServerAuthorizationCheckInterface {
+ int Schedule(ge::TlsServerAuthorizationCheckArg* arg) override {
+ arg->set_success(1);
+ arg->set_status(GRPC_STATUS_OK);
+ return 0;
+ }
+ };
+
+ noop_auth_check_ = std::make_shared<ge::TlsServerAuthorizationCheckConfig>(
+ 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(
Review comment:
So gRPC has both `TlsCredentialsOptions` and `SslCredentialOptions`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jduo commented on pull request #8325: ARROW-10206: [C++][Python][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
jduo commented on pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#issuecomment-705342986
I've implemented auto-detection of the namespace of TlsCredentialsOptions now. This should let the code work across multiple gRPC versions. If TlsCredentialsOptions doesn't exist, use of it will get complied out.
Also switched the code to only require 1.27 at the minimum, and reverted changes where gRPC > 1.27
was already being used.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jduo commented on a change in pull request #8325: ARROW-10206: [C++][Python][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
jduo commented on a change in pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#discussion_r502531900
##########
File path: cpp/src/arrow/flight/client.cc
##########
@@ -845,18 +878,48 @@ class FlightClient::FlightClientImpl {
if (scheme == kSchemeGrpc || scheme == kSchemeGrpcTcp || scheme == kSchemeGrpcTls) {
grpc_uri << location.uri_->host() << ":" << location.uri_->port_text();
- if (scheme == "grpc+tls") {
- grpc::SslCredentialsOptions ssl_options;
- if (!options.tls_root_certs.empty()) {
- ssl_options.pem_root_certs = options.tls_root_certs;
- }
- if (!options.cert_chain.empty()) {
- ssl_options.pem_cert_chain = options.cert_chain;
- }
- if (!options.private_key.empty()) {
- ssl_options.pem_private_key = options.private_key;
+ if (scheme == kSchemeGrpcTls) {
+ if (options.disable_server_verification) {
+#if !defined(GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS)
+ return Status::NotImplemented(
+ "Using encryption with server verification is unsupported.");
+#else
+ namespace ge = GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS;
+
+ // A callback to supply to TlsCredentialsOptions that accepts any server
+ // arguments.
+ struct NoOpTlsAuthorizationCheck
+ : public ge::TlsServerAuthorizationCheckInterface {
+ int Schedule(ge::TlsServerAuthorizationCheckArg* arg) override {
+ arg->set_success(1);
+ arg->set_status(GRPC_STATUS_OK);
+ return 0;
+ }
+ };
+
+ noop_auth_check_ = std::make_shared<ge::TlsServerAuthorizationCheckConfig>(
+ 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(
Review comment:
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.
##########
File path: cpp/src/arrow/flight/client.cc
##########
@@ -845,18 +878,48 @@ class FlightClient::FlightClientImpl {
if (scheme == kSchemeGrpc || scheme == kSchemeGrpcTcp || scheme == kSchemeGrpcTls) {
grpc_uri << location.uri_->host() << ":" << location.uri_->port_text();
- if (scheme == "grpc+tls") {
- grpc::SslCredentialsOptions ssl_options;
- if (!options.tls_root_certs.empty()) {
- ssl_options.pem_root_certs = options.tls_root_certs;
- }
- if (!options.cert_chain.empty()) {
- ssl_options.pem_cert_chain = options.cert_chain;
- }
- if (!options.private_key.empty()) {
- ssl_options.pem_private_key = options.private_key;
+ if (scheme == kSchemeGrpcTls) {
+ if (options.disable_server_verification) {
+#if !defined(GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS)
+ return Status::NotImplemented(
+ "Using encryption with server verification is unsupported.");
Review comment:
Done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm edited a comment on pull request #8325: ARROW-10206: [C++][Python][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
wesm edited a comment on pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#issuecomment-705851235
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.
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] emkornfield commented on a change in pull request #8325: ARROW-10206: [C++][Python][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#discussion_r502156886
##########
File path: cpp/src/arrow/flight/client.cc
##########
@@ -835,6 +843,31 @@ class GrpcMetadataReader : public FlightMetadataReader {
std::shared_ptr<std::mutex> read_mutex_;
};
+namespace {
+// Dummy self-signed certificate to be used because TlsCredentials
+// requires root CA certs, even if you are skipping server
+// verification.
+#if defined(GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS)
+const char BLANK_ROOT_PEM[] =
Review comment:
nit: constexpr? or static.
##########
File path: cpp/src/arrow/flight/client.h
##########
@@ -90,6 +90,8 @@ class ARROW_FLIGHT_EXPORT FlightWriteSizeStatusDetail : public arrow::StatusDeta
class ARROW_FLIGHT_EXPORT FlightClientOptions {
public:
+ FlightClientOptions();
Review comment:
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #8325: ARROW-10206: [C++][Python][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#discussion_r502338793
##########
File path: cpp/src/arrow/flight/client.cc
##########
@@ -845,18 +878,48 @@ class FlightClient::FlightClientImpl {
if (scheme == kSchemeGrpc || scheme == kSchemeGrpcTcp || scheme == kSchemeGrpcTls) {
grpc_uri << location.uri_->host() << ":" << location.uri_->port_text();
- if (scheme == "grpc+tls") {
- grpc::SslCredentialsOptions ssl_options;
- if (!options.tls_root_certs.empty()) {
- ssl_options.pem_root_certs = options.tls_root_certs;
- }
- if (!options.cert_chain.empty()) {
- ssl_options.pem_cert_chain = options.cert_chain;
- }
- if (!options.private_key.empty()) {
- ssl_options.pem_private_key = options.private_key;
+ if (scheme == kSchemeGrpcTls) {
+ if (options.disable_server_verification) {
+#if !defined(GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS)
+ return Status::NotImplemented(
+ "Using encryption with server verification is unsupported.");
Review comment:
Mention the reason in the error message?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a change in pull request #8325: ARROW-10105: [C++][Python][Java][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
lidavidm commented on a change in pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#discussion_r499662175
##########
File path: cpp/src/arrow/flight/client.h
##########
@@ -108,10 +108,14 @@ class ARROW_FLIGHT_EXPORT FlightClientOptions {
/// positive. When enabled, FlightStreamWriter.Write* may yield a
/// IOError with error detail FlightWriteSizeStatusDetail.
int64_t write_size_limit_bytes;
+
/// \brief Generic connection options, passed to the underlying
/// transport; interpretation is implementation-dependent.
std::vector<std::pair<std::string, util::variant<int, std::string>>> generic_options;
+ /// \brief Use TLS without validating the server certificate. Use with caution.
+ bool disable_server_verification;
+
/// \brief Get default options.
static FlightClientOptions Defaults();
Review comment:
The intent was to use Defaults(), at least.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm commented on a change in pull request #8325: ARROW-10206: [C++][Python][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
wesm commented on a change in pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#discussion_r502002471
##########
File path: cpp/src/arrow/flight/client.cc
##########
@@ -845,18 +878,52 @@ class FlightClient::FlightClientImpl {
if (scheme == kSchemeGrpc || scheme == kSchemeGrpcTcp || scheme == kSchemeGrpcTls) {
grpc_uri << location.uri_->host() << ":" << location.uri_->port_text();
- if (scheme == "grpc+tls") {
- grpc::SslCredentialsOptions ssl_options;
- if (!options.tls_root_certs.empty()) {
- ssl_options.pem_root_certs = options.tls_root_certs;
- }
- if (!options.cert_chain.empty()) {
- ssl_options.pem_cert_chain = options.cert_chain;
- }
- if (!options.private_key.empty()) {
- ssl_options.pem_private_key = options.private_key;
+ if (scheme == kSchemeGrpcTls) {
+ if (options.disable_server_verification) {
+#if !defined(GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS)
+ return Status::NotImplemented(
+ "Using encryption with server verification is unsupported.");
+#else
+ namespace ge = GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS;
+
+ // A callback to supply to TlsCredentialsOptions that accepts any server
+ // arguments.
+ struct NoOpTlsAuthorizationCheck
+ : public ge::TlsServerAuthorizationCheckInterface {
+ int Schedule(ge::TlsServerAuthorizationCheckArg* arg) override {
+ arg->set_success(1);
+ arg->set_status(GRPC_STATUS_OK);
+ return 0;
+ }
+ };
+
+ noOpAuthCheck = std::shared_ptr<ge::TlsServerAuthorizationCheckConfig>(
Review comment:
Use `no_op_auth_check_` (or `noop_auth_check_`, whichever is more consistent with how we write it elsewhere in the codebase)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm commented on a change in pull request #8325: ARROW-10206: [C++][Python][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
wesm commented on a change in pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#discussion_r502002828
##########
File path: cpp/src/arrow/flight/client.cc
##########
@@ -845,18 +878,52 @@ class FlightClient::FlightClientImpl {
if (scheme == kSchemeGrpc || scheme == kSchemeGrpcTcp || scheme == kSchemeGrpcTls) {
grpc_uri << location.uri_->host() << ":" << location.uri_->port_text();
- if (scheme == "grpc+tls") {
- grpc::SslCredentialsOptions ssl_options;
- if (!options.tls_root_certs.empty()) {
- ssl_options.pem_root_certs = options.tls_root_certs;
- }
- if (!options.cert_chain.empty()) {
- ssl_options.pem_cert_chain = options.cert_chain;
- }
- if (!options.private_key.empty()) {
- ssl_options.pem_private_key = options.private_key;
+ if (scheme == kSchemeGrpcTls) {
+ if (options.disable_server_verification) {
+#if !defined(GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS)
+ return Status::NotImplemented(
+ "Using encryption with server verification is unsupported.");
+#else
+ namespace ge = GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS;
+
+ // A callback to supply to TlsCredentialsOptions that accepts any server
+ // arguments.
+ struct NoOpTlsAuthorizationCheck
+ : public ge::TlsServerAuthorizationCheckInterface {
+ int Schedule(ge::TlsServerAuthorizationCheckArg* arg) override {
+ arg->set_success(1);
+ arg->set_status(GRPC_STATUS_OK);
+ return 0;
+ }
+ };
+
+ noOpAuthCheck = std::shared_ptr<ge::TlsServerAuthorizationCheckConfig>(
+ new ge::TlsServerAuthorizationCheckConfig(
Review comment:
Can you use `make_shared` here?
##########
File path: cpp/src/arrow/flight/client.cc
##########
@@ -845,18 +878,52 @@ class FlightClient::FlightClientImpl {
if (scheme == kSchemeGrpc || scheme == kSchemeGrpcTcp || scheme == kSchemeGrpcTls) {
grpc_uri << location.uri_->host() << ":" << location.uri_->port_text();
- if (scheme == "grpc+tls") {
- grpc::SslCredentialsOptions ssl_options;
- if (!options.tls_root_certs.empty()) {
- ssl_options.pem_root_certs = options.tls_root_certs;
- }
- if (!options.cert_chain.empty()) {
- ssl_options.pem_cert_chain = options.cert_chain;
- }
- if (!options.private_key.empty()) {
- ssl_options.pem_private_key = options.private_key;
+ if (scheme == kSchemeGrpcTls) {
+ if (options.disable_server_verification) {
+#if !defined(GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS)
+ return Status::NotImplemented(
+ "Using encryption with server verification is unsupported.");
+#else
+ namespace ge = GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS;
+
+ // A callback to supply to TlsCredentialsOptions that accepts any server
+ // arguments.
+ struct NoOpTlsAuthorizationCheck
+ : public ge::TlsServerAuthorizationCheckInterface {
+ int Schedule(ge::TlsServerAuthorizationCheckArg* arg) override {
+ arg->set_success(1);
+ arg->set_status(GRPC_STATUS_OK);
+ return 0;
+ }
+ };
+
+ noOpAuthCheck = std::shared_ptr<ge::TlsServerAuthorizationCheckConfig>(
Review comment:
Use `no_op_auth_check_`
##########
File path: cpp/src/arrow/flight/client.cc
##########
@@ -845,18 +878,52 @@ class FlightClient::FlightClientImpl {
if (scheme == kSchemeGrpc || scheme == kSchemeGrpcTcp || scheme == kSchemeGrpcTls) {
grpc_uri << location.uri_->host() << ":" << location.uri_->port_text();
- if (scheme == "grpc+tls") {
- grpc::SslCredentialsOptions ssl_options;
- if (!options.tls_root_certs.empty()) {
- ssl_options.pem_root_certs = options.tls_root_certs;
- }
- if (!options.cert_chain.empty()) {
- ssl_options.pem_cert_chain = options.cert_chain;
- }
- if (!options.private_key.empty()) {
- ssl_options.pem_private_key = options.private_key;
+ if (scheme == kSchemeGrpcTls) {
+ if (options.disable_server_verification) {
+#if !defined(GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS)
+ return Status::NotImplemented(
+ "Using encryption with server verification is unsupported.");
+#else
+ namespace ge = GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS;
+
+ // A callback to supply to TlsCredentialsOptions that accepts any server
+ // arguments.
+ struct NoOpTlsAuthorizationCheck
+ : public ge::TlsServerAuthorizationCheckInterface {
+ int Schedule(ge::TlsServerAuthorizationCheckArg* arg) override {
+ arg->set_success(1);
+ arg->set_status(GRPC_STATUS_OK);
+ return 0;
+ }
+ };
+
+ noOpAuthCheck = std::shared_ptr<ge::TlsServerAuthorizationCheckConfig>(
+ new ge::TlsServerAuthorizationCheckConfig(
+ std::shared_ptr<ge::TlsServerAuthorizationCheckInterface>(
+ new NoOpTlsAuthorizationCheck())));
+ std::shared_ptr<ge::TlsKeyMaterialsConfig> materials_config(
+ new ge::TlsKeyMaterialsConfig());
Review comment:
`auto materials_config = std::make_shared<ge::TlsKeyMaterialsConfig>();`?
##########
File path: cpp/src/arrow/flight/client.cc
##########
@@ -845,18 +878,52 @@ class FlightClient::FlightClientImpl {
if (scheme == kSchemeGrpc || scheme == kSchemeGrpcTcp || scheme == kSchemeGrpcTls) {
grpc_uri << location.uri_->host() << ":" << location.uri_->port_text();
- if (scheme == "grpc+tls") {
- grpc::SslCredentialsOptions ssl_options;
- if (!options.tls_root_certs.empty()) {
- ssl_options.pem_root_certs = options.tls_root_certs;
- }
- if (!options.cert_chain.empty()) {
- ssl_options.pem_cert_chain = options.cert_chain;
- }
- if (!options.private_key.empty()) {
- ssl_options.pem_private_key = options.private_key;
+ if (scheme == kSchemeGrpcTls) {
+ if (options.disable_server_verification) {
+#if !defined(GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS)
+ return Status::NotImplemented(
+ "Using encryption with server verification is unsupported.");
+#else
+ namespace ge = GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS;
+
+ // A callback to supply to TlsCredentialsOptions that accepts any server
+ // arguments.
+ struct NoOpTlsAuthorizationCheck
+ : public ge::TlsServerAuthorizationCheckInterface {
+ int Schedule(ge::TlsServerAuthorizationCheckArg* arg) override {
+ arg->set_success(1);
+ arg->set_status(GRPC_STATUS_OK);
+ return 0;
+ }
+ };
+
+ noOpAuthCheck = std::shared_ptr<ge::TlsServerAuthorizationCheckConfig>(
+ new ge::TlsServerAuthorizationCheckConfig(
+ std::shared_ptr<ge::TlsServerAuthorizationCheckInterface>(
+ new NoOpTlsAuthorizationCheck())));
Review comment:
possible to use `make_shared`?
##########
File path: python/pyarrow/_flight.pyx
##########
@@ -1106,13 +1114,15 @@ cdef class FlightClient(_Weakrefable):
@classmethod
def connect(cls, location, tls_root_certs=None, cert_chain=None,
- private_key=None, override_hostname=None):
+ private_key=None, override_hostname=None,
+ disable_server_verify=None):
Review comment:
Make this consistent? verify -> verification
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jduo commented on pull request #8325: ARROW-10206: [C++][Python][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
jduo commented on pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#issuecomment-705342986
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jduo commented on a change in pull request #8325: ARROW-10206: [C++][Python][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
jduo commented on a change in pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#discussion_r501460070
##########
File path: python/manylinux1/scripts/build_re2.sh
##########
@@ -31,5 +31,7 @@ make prefix=/usr/local -j${NCORES} install
popd
-# Need to remove shared library to make sure the static library is picked up by Arrow
-rm -rf re2-${RE2_VERSION}.tar.gz re2-${RE2_VERSION} /usr/local/lib/libre2.so*
+# Need to remove static library to make sure the shared library is picked up by Arrow
+# The static library fails to link when using gRPC 1.32.
+rm -rf re2-${RE2_VERSION}.tar.gz re2-${RE2_VERSION} /usr/local/lib/libre2.a
Review comment:
Reverted since we only need gRPC 1.27.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jduo commented on a change in pull request #8325: ARROW-10206: [C++][Python][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
jduo commented on a change in pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#discussion_r502020136
##########
File path: cpp/src/arrow/flight/client.cc
##########
@@ -845,18 +878,52 @@ class FlightClient::FlightClientImpl {
if (scheme == kSchemeGrpc || scheme == kSchemeGrpcTcp || scheme == kSchemeGrpcTls) {
grpc_uri << location.uri_->host() << ":" << location.uri_->port_text();
- if (scheme == "grpc+tls") {
- grpc::SslCredentialsOptions ssl_options;
- if (!options.tls_root_certs.empty()) {
- ssl_options.pem_root_certs = options.tls_root_certs;
- }
- if (!options.cert_chain.empty()) {
- ssl_options.pem_cert_chain = options.cert_chain;
- }
- if (!options.private_key.empty()) {
- ssl_options.pem_private_key = options.private_key;
+ if (scheme == kSchemeGrpcTls) {
+ if (options.disable_server_verification) {
+#if !defined(GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS)
+ return Status::NotImplemented(
+ "Using encryption with server verification is unsupported.");
+#else
+ namespace ge = GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS;
+
+ // A callback to supply to TlsCredentialsOptions that accepts any server
+ // arguments.
+ struct NoOpTlsAuthorizationCheck
+ : public ge::TlsServerAuthorizationCheckInterface {
+ int Schedule(ge::TlsServerAuthorizationCheckArg* arg) override {
+ arg->set_success(1);
+ arg->set_status(GRPC_STATUS_OK);
+ return 0;
+ }
+ };
+
+ noOpAuthCheck = std::shared_ptr<ge::TlsServerAuthorizationCheckConfig>(
Review comment:
Done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #8325: ARROW-10105: [C++][Python][Java][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#issuecomment-702709693
Actually, it seems the relevant changes have been there since early this year. It may be OK to switch fully.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jduo commented on a change in pull request #8325: ARROW-10206: [C++][Python][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
jduo commented on a change in pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#discussion_r501186324
##########
File path: python/manylinux1/scripts/build_re2.sh
##########
@@ -31,5 +31,7 @@ make prefix=/usr/local -j${NCORES} install
popd
-# Need to remove shared library to make sure the static library is picked up by Arrow
-rm -rf re2-${RE2_VERSION}.tar.gz re2-${RE2_VERSION} /usr/local/lib/libre2.so*
+# Need to remove static library to make sure the shared library is picked up by Arrow
+# The static library fails to link when using gRPC 1.32.
+rm -rf re2-${RE2_VERSION}.tar.gz re2-${RE2_VERSION} /usr/local/lib/libre2.a
Review comment:
I was seeing linker errors and wasn't aware static linking was a hard requirement for manylinux1.
The linker error was the the string constructor for RE2 was not being defined.
Any thoughts about how to fix this? Flight doesn't use RE2 directly -- gRPC uses RE2. For this type of problem, with GNU Makefiles I'd change the order libraries were linked in to ensure gRPC is fed to the linker before RE2 or add the linker flag to disable stripping of unneeded symbols. I'm not sure about doing this with CMake though.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #8325: ARROW-10206: [C++][Python][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#issuecomment-704796210
https://issues.apache.org/jira/browse/ARROW-10206
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jduo commented on a change in pull request #8325: ARROW-10206: [C++][Python][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
jduo commented on a change in pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#discussion_r502020189
##########
File path: cpp/src/arrow/flight/client.cc
##########
@@ -845,18 +878,52 @@ class FlightClient::FlightClientImpl {
if (scheme == kSchemeGrpc || scheme == kSchemeGrpcTcp || scheme == kSchemeGrpcTls) {
grpc_uri << location.uri_->host() << ":" << location.uri_->port_text();
- if (scheme == "grpc+tls") {
- grpc::SslCredentialsOptions ssl_options;
- if (!options.tls_root_certs.empty()) {
- ssl_options.pem_root_certs = options.tls_root_certs;
- }
- if (!options.cert_chain.empty()) {
- ssl_options.pem_cert_chain = options.cert_chain;
- }
- if (!options.private_key.empty()) {
- ssl_options.pem_private_key = options.private_key;
+ if (scheme == kSchemeGrpcTls) {
+ if (options.disable_server_verification) {
+#if !defined(GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS)
+ return Status::NotImplemented(
+ "Using encryption with server verification is unsupported.");
+#else
+ namespace ge = GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS;
+
+ // A callback to supply to TlsCredentialsOptions that accepts any server
+ // arguments.
+ struct NoOpTlsAuthorizationCheck
+ : public ge::TlsServerAuthorizationCheckInterface {
+ int Schedule(ge::TlsServerAuthorizationCheckArg* arg) override {
+ arg->set_success(1);
+ arg->set_status(GRPC_STATUS_OK);
+ return 0;
+ }
+ };
+
+ noOpAuthCheck = std::shared_ptr<ge::TlsServerAuthorizationCheckConfig>(
+ new ge::TlsServerAuthorizationCheckConfig(
+ std::shared_ptr<ge::TlsServerAuthorizationCheckInterface>(
+ new NoOpTlsAuthorizationCheck())));
Review comment:
Done
##########
File path: cpp/src/arrow/flight/client.cc
##########
@@ -845,18 +878,52 @@ class FlightClient::FlightClientImpl {
if (scheme == kSchemeGrpc || scheme == kSchemeGrpcTcp || scheme == kSchemeGrpcTls) {
grpc_uri << location.uri_->host() << ":" << location.uri_->port_text();
- if (scheme == "grpc+tls") {
- grpc::SslCredentialsOptions ssl_options;
- if (!options.tls_root_certs.empty()) {
- ssl_options.pem_root_certs = options.tls_root_certs;
- }
- if (!options.cert_chain.empty()) {
- ssl_options.pem_cert_chain = options.cert_chain;
- }
- if (!options.private_key.empty()) {
- ssl_options.pem_private_key = options.private_key;
+ if (scheme == kSchemeGrpcTls) {
+ if (options.disable_server_verification) {
+#if !defined(GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS)
+ return Status::NotImplemented(
+ "Using encryption with server verification is unsupported.");
+#else
+ namespace ge = GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS;
+
+ // A callback to supply to TlsCredentialsOptions that accepts any server
+ // arguments.
+ struct NoOpTlsAuthorizationCheck
+ : public ge::TlsServerAuthorizationCheckInterface {
+ int Schedule(ge::TlsServerAuthorizationCheckArg* arg) override {
+ arg->set_success(1);
+ arg->set_status(GRPC_STATUS_OK);
+ return 0;
+ }
+ };
+
+ noOpAuthCheck = std::shared_ptr<ge::TlsServerAuthorizationCheckConfig>(
+ new ge::TlsServerAuthorizationCheckConfig(
+ std::shared_ptr<ge::TlsServerAuthorizationCheckInterface>(
+ new NoOpTlsAuthorizationCheck())));
+ std::shared_ptr<ge::TlsKeyMaterialsConfig> materials_config(
+ new ge::TlsKeyMaterialsConfig());
Review comment:
Done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jduo commented on pull request #8325: ARROW-10206: [C++][Python][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
jduo commented on pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#issuecomment-704792871
I've changed this PR to cover just C++ and Python.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm commented on pull request #8325: ARROW-10206: [C++][Python][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
wesm commented on pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#issuecomment-705851235
There's some broken stuff for me locally with clang-8
```
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.
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm commented on pull request #8325: ARROW-10206: [C++][Python][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
wesm commented on pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#issuecomment-706311621
thanks everyone
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm edited a comment on pull request #8325: ARROW-10206: [C++][Python][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
wesm edited a comment on pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#issuecomment-705851235
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.
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jduo commented on pull request #8325: ARROW-10206: [C++][Python][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
jduo commented on pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#issuecomment-705155690
FYI @nealrichardson @pitrou @wesm
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kszucs commented on a change in pull request #8325: ARROW-10206: [C++][Python][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
kszucs commented on a change in pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#discussion_r501307054
##########
File path: python/manylinux1/scripts/build_grpc.sh
##########
@@ -16,8 +16,61 @@
# specific language governing permissions and limitations
# under the License.
-export GRPC_VERSION="1.29.1"
-export CFLAGS="-fPIC -DGPR_MANYLINUX1=1"
+export GRPC_VERSION="1.32.0"
Review comment:
Please update `python/manylinux201x/scripts/build_grpc.sh` as well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jduo commented on a change in pull request #8325: ARROW-10206: [C++][Python][FlightRPC] Allow disabling server validation
Posted by GitBox <gi...@apache.org>.
jduo commented on a change in pull request #8325:
URL: https://github.com/apache/arrow/pull/8325#discussion_r502248222
##########
File path: cpp/src/arrow/flight/client.cc
##########
@@ -835,6 +843,31 @@ class GrpcMetadataReader : public FlightMetadataReader {
std::shared_ptr<std::mutex> read_mutex_;
};
+namespace {
+// Dummy self-signed certificate to be used because TlsCredentials
+// requires root CA certs, even if you are skipping server
+// verification.
+#if defined(GRPC_NAMESPACE_FOR_TLS_CREDENTIALS_OPTIONS)
+const char BLANK_ROOT_PEM[] =
Review comment:
Made a constexpr
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org