ARROW-10487 [FlightRPC][C++] Header-based auth in clients #8724

lyndonbauto · 2020-11-20T17:31:24Z

Added support for header based authentication in clients.

Added support for base 64 encoded username / password auth to match Java implementation
Added bearer token receiving and populating of call options to send back
Added unit tests that connects C++ client and a mock C++ server to test authentication

…d server header authentication in Java

…lyndonb-bq/arrow into jduo/lyndon/flight-auth-cpp-redesign-client

github-actions · 2020-11-20T17:32:05Z

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

lyndonbauto · 2020-11-20T18:16:32Z

@lidavidm
Hi David,
This is currently a work in progress. I was wondering if you could take a look at how this was integrated into the C++ library and let me know if it is okay or if there is a better way to do it? Afterwards I will make any required code changes and fix the code linting/commenting.
Thanks!

kylepbit · 2020-11-20T20:59:30Z

cpp/src/arrow/flight/client.cc

@@ -51,6 +51,7 @@

 #include "arrow/flight/client_auth.h"
 #include "arrow/flight/client_middleware.h"
+#include "arrow/flight/client_header_auth_middleware.h"


Is this ordering alphabetical? If so this should be up one...

Yes, was waiting for feedback on design before proper formatting. I will correct this.

kylepbit · 2020-11-20T21:00:01Z

cpp/src/arrow/flight/client.cc

@@ -328,7 +332,7 @@ class GrpcClientInterceptorAdapterFactory
    : public grpc::experimental::ClientInterceptorFactoryInterface {
 public:
  GrpcClientInterceptorAdapterFactory(
-      std::vector<std::shared_ptr<ClientMiddlewareFactory>> middleware)
+      std::vector<std::shared_ptr<ClientMiddlewareFactory>>& middleware)


Should this be const?

This should either be const reference or a pointer.

So, I'd rather we store a reference to this interceptor factory instead of a reference to the middleware with a mutable pointer back, i.e. I'd rather have this class own the middleware as it currently does, and have FlightClient call a method of this class to add more middleware at runtime.

kylepbit · 2020-11-20T21:00:20Z

cpp/src/arrow/flight/client.cc

@@ -371,7 +375,7 @@ class GrpcClientInterceptorAdapterFactory
  }

 private:
-  std::vector<std::shared_ptr<ClientMiddlewareFactory>> middleware_;
+  std::vector<std::shared_ptr<ClientMiddlewareFactory>>& middleware_;


Intention is to keep the reference and not a copy?

This should be a pointer if not a copy.

It was to keep the reference, but you got me thinking more and I think I should just take a copy and then make a function to allow adding to the middleware on the fly, since the issue is that I need to be able to add and remove middleware from the interceptor. I will rejig this.

kylepbit · 2020-11-20T21:00:36Z

cpp/src/arrow/flight/client.cc

@@ -963,8 +967,9 @@ class FlightClient::FlightClientImpl {

    std::vector<std::unique_ptr<grpc::experimental::ClientInterceptorFactoryInterface>>
        interceptors;
+    middleware = std::move(options.middleware);


Why this change?

Goal was so that middleware was retained in this class so I could push and pop it. I am going to adjust the implementation though.

kylepbit · 2020-11-20T21:01:10Z

cpp/src/arrow/flight/client.cc

@@ -993,6 +998,30 @@ class FlightClient::FlightClientImpl {
    return Status::OK();
  }

+  Status AuthenticateBasicToken(std::string username, std::string password,


Should username/password be passed by const ref?

kylepbit · 2020-11-20T21:01:49Z

cpp/src/arrow/flight/client.cc

+  Status AuthenticateBasicToken(std::string username, std::string password,
+                                std::pair<std::string, std::string>* bearer_token) {
+    // Add bearer token factory to middleware so it can intercept the bearer token.
+    middleware.push_back(std::make_shared<ClientBearerTokenFactory>(bearer_token));


This looks odd to create the shared pointer after you've passed in the raw pointer....it seems like the method itself should take a shared pointer.

The raw pointer is unpopulated, so it's passed to the BearerTokenFactory's constructor, which stores it and populated it when it receives the bearer token. I could make the client pass the whole factory in with the bearer token already inside it, but it's more work and requires they understand what's going on more than they otherwise would need to.

kylepbit · 2020-11-20T21:02:59Z

cpp/src/arrow/flight/client_header_auth_middleware.cc

+  void ClientBearerTokenMiddleware::ReceivedHeaders(
+    const CallHeaders& incoming_headers) {
+    // Grab the auth token if one exists.
+    auto bearer_iter = incoming_headers.find(AUTH_HEADER);


kylepbit · 2020-11-20T21:03:16Z

cpp/src/arrow/flight/client_header_auth_middleware.cc

+    // Check if the value of the auth token starts with the bearer prefix, latch the token.
+    std::string bearer_val = bearer_iter->second.to_string();
+    if (bearer_val.size() > BEARER_PREFIX.size()) {
+      bool hasPrefix = std::equal(bearer_val.begin(), bearer_val.begin() + BEARER_PREFIX.size(), BEARER_PREFIX.begin(),


Seems like they use snake_case instead of camelCase.

kylepbit · 2020-11-20T21:03:49Z

cpp/src/arrow/flight/client_header_auth_middleware.cc

+  void ClientBearerTokenMiddleware::CallCompleted(const Status& status) { }
+
+  void ClientBearerTokenFactory::StartCall(const CallInfo& info, std::unique_ptr<ClientMiddleware>* middleware) {
+    *middleware = std::unique_ptr<ClientBearerTokenMiddleware>(new ClientBearerTokenMiddleware(bearer_token));


std::make_unique?

kylepbit · 2020-11-20T21:04:16Z

cpp/src/arrow/flight/client_header_auth_middleware.cc

+
+  void ClientBearerTokenMiddleware::CallCompleted(const Status& status) { }
+
+  void ClientBearerTokenFactory::StartCall(const CallInfo& info, std::unique_ptr<ClientMiddleware>* middleware) {


Would it be better to pass a reference instead of a pointer?

Can't this is a method of the base class, I think it's done this way to allow you to assign a new unique pointer to it without exposing the other middlewares they are already holding in their vector.

kylepbit · 2020-11-20T21:04:43Z

cpp/src/arrow/flight/client_header_auth_middleware.cc

+  std::string string_format(const std::string& format, const Args... args) {
+    // Check size requirement for new string and increment by 1 for null terminator.
+    size_t size = std::snprintf(nullptr, 0, format.c_str(), args ...) + 1;
+    if(size <= 0){


Nit: spacing between if (, ){

kylepbit · 2020-11-20T21:05:17Z

cpp/src/arrow/flight/client_header_auth_middleware.cc

+  }
+
+  std::string base64_encode(const std::string& input) {
+     static const std::string base64_chars =


This doesn't exist in the codebase already?

Yes, I found it. Will remove this.

lidavidm · 2020-11-20T21:06:35Z

Note, for formatting, use clang-format; for naming conventions, please follow Google C++ guidelines. See the developer docs.

In particular, use snake_case, use pointers instead of non-const references, and note that constants are named kCamelCase not UPPER_SNAKE_CASE.

kylepbit · 2020-11-20T21:06:37Z

cpp/src/arrow/flight/client_header_auth_middleware.h

+#include <cctype>
+#include <string>
+
+const std::string AUTH_HEADER = "authorization";


Do these need to be in the header, or should they be only in the CC.
If in the header, I think they're created anew for each compilation unit, and thus should be defined as extern with the actual value defined in the CC to avoid multiple instantiations.

lidavidm

Some initial comments.

Also:

We should have unit tests, not just integration tests
If you want this to actually run as part of the integration suite, you'll have to add it to the list of test scenarios and various other places; maybe take a look at the commit:
0188e45

lidavidm · 2020-11-20T21:07:55Z

cpp/src/arrow/flight/client.cc

@@ -328,7 +332,7 @@ class GrpcClientInterceptorAdapterFactory
    : public grpc::experimental::ClientInterceptorFactoryInterface {
 public:
  GrpcClientInterceptorAdapterFactory(
-      std::vector<std::shared_ptr<ClientMiddlewareFactory>> middleware)
+      std::vector<std::shared_ptr<ClientMiddlewareFactory>>& middleware)


This should either be const reference or a pointer.

lidavidm · 2020-11-20T21:08:07Z

cpp/src/arrow/flight/client.cc

@@ -371,7 +375,7 @@ class GrpcClientInterceptorAdapterFactory
  }

 private:
-  std::vector<std::shared_ptr<ClientMiddlewareFactory>> middleware_;
+  std::vector<std::shared_ptr<ClientMiddlewareFactory>>& middleware_;


This should be a pointer if not a copy.

lidavidm · 2020-11-20T21:14:09Z

cpp/src/arrow/flight/client.cc

@@ -328,7 +332,7 @@ class GrpcClientInterceptorAdapterFactory
    : public grpc::experimental::ClientInterceptorFactoryInterface {
 public:
  GrpcClientInterceptorAdapterFactory(
-      std::vector<std::shared_ptr<ClientMiddlewareFactory>> middleware)
+      std::vector<std::shared_ptr<ClientMiddlewareFactory>>& middleware)


So, I'd rather we store a reference to this interceptor factory instead of a reference to the middleware with a mutable pointer back, i.e. I'd rather have this class own the middleware as it currently does, and have FlightClient call a method of this class to add more middleware at runtime.

lidavidm · 2020-11-20T21:15:20Z

cpp/src/arrow/flight/client.h

@@ -65,6 +65,9 @@ class ARROW_FLIGHT_EXPORT FlightCallOptions {

  /// \brief IPC writer options, if applicable for the call.
  ipc::IpcWriteOptions write_options;
+
+  /// \brief Metadata for client to add to context.
+  std::vector<std::pair<std::string, std::string>> metadata;


I believe we call it headers elsewhere, so this should stay consistent with that.

Will correct this.

lidavidm · 2020-11-20T21:15:34Z

cpp/src/arrow/flight/client.h

@@ -191,6 +194,14 @@ class ARROW_FLIGHT_EXPORT FlightClient {
  Status Authenticate(const FlightCallOptions& options,
                      std::unique_ptr<ClientAuthHandler> auth_handler);

+  /// \brief Authenticate to the server using the given handler.


There's no handler in play here.

lidavidm · 2020-11-20T21:17:23Z

cpp/src/arrow/flight/client_header_auth_middleware.h

+#include <cctype>
+#include <string>
+
+const std::string AUTH_HEADER = "authorization";


These should be named kAuthHeader etc. and should go in the .cc file unless we actually want to expose these to users.

Or alternatively, the other constants are currently in one of the _internal.h headers.

lidavidm · 2020-11-20T21:18:13Z

cpp/src/arrow/flight/client_header_auth_middleware.h

+namespace flight {
+
+// TODO: Need to add documentation in this file.
+void ARROW_FLIGHT_EXPORT AddBasicAuthHeaders(grpc::ClientContext* context, 


This is internal - it shouldn't be in a public header. (Ditto for the grpc include.)

lidavidm · 2020-11-20T21:19:15Z

cpp/src/arrow/flight/client_header_auth_middleware.h

@@ -0,0 +1,78 @@
+// Licensed to the Apache Software Foundation (ASF) under one


This should probably be an _internal.h header since I don't think any of this is intended to be directly used outside of the implementation here.

lidavidm · 2020-11-20T21:24:28Z

cpp/src/arrow/flight/client_header_auth_middleware.cc

+    *bearer_token = std::make_pair("", "");
+  }
+
+  template<typename ... Args>


I don't think we need an entire templated function to concatenate two strings.

lidavidm · 2020-11-20T21:24:43Z

cpp/src/arrow/flight/client_header_auth_middleware.cc

+    // Check size requirement for new string and increment by 1 for null terminator.
+    size_t size = std::snprintf(nullptr, 0, format.c_str(), args ...) + 1;
+    if(size <= 0){
+      throw std::runtime_error("Error during string formatting. Format: '" + format + "'.");


And Arrow disallows exceptions.

…round unit tests but checking in WIP.

lidavidm · 2020-11-24T12:37:14Z

By the way, for CMake formatting, please use cmake_format

[2] Fixed option passing.

…ndon/flight-auth-cpp-redesign-client

…ilds.

lidavidm

LGTM - just two minor comments (one about the interface, one about a test)

lidavidm · 2020-11-25T17:30:51Z

cpp/src/arrow/flight/client.cc

@@ -1198,6 +1227,12 @@ Status FlightClient::Authenticate(const FlightCallOptions& options,
  return impl_->Authenticate(options, std::move(auth_handler));
 }

+Status FlightClient::AuthenticateBasicToken(


Could we make this arrow::Result<std::pair<>>?

Yep, I will make this change.

lidavidm · 2020-11-25T17:36:25Z

cpp/src/arrow/flight/flight_test.cc

+    std::pair<std::string, std::string> bearer_token;
+    // Note: Status intentionally ignored because it requires C++ server implementation of
+    // header auth. For now it returns an IOError.
+    arrow::Status status = client_->AuthenticateBasicToken(


If the intent is to test that this fails, we can ASSERT_RAISES(IOError, status) - even if we had an implementation of header auth, we'd want this to fail, right?

Sorry, this was an artifact of my initial test implementation which was incorrect on the test server, and always caused an error. I forgot to remove it from the negative test once I fixed it. I will correct this.

[2] Switched return type of AuthenticateBasicToken to use arrow::Result instead of Status and removed bearer token from parameter list

lyndonbauto · 2020-11-25T20:07:51Z

@lidavidm Hey David, I have made the requested changes. I got a random Javascript failure in one of the CI teardowns, not sure if this is accepted or if you can restart the CI build (I do not have permission to restart it myself).

lyndonbauto · 2020-11-25T20:50:56Z

@lidavidm Hey David, I have made the requested changes. I got a random Javascript failure in one of the CI teardowns, not sure if this is accepted or if you can restart the CI build (I do not have permission to restart it myself).

@lidavidm All tests are now passing 🎉

lidavidm · 2020-11-25T21:01:32Z

Thanks @lyndonb-bq!

Added support for header based authentication in clients. - Added support for base 64 encoded username / password auth to match Java implementation - Added bearer token receiving and populating of call options to send back - Added unit tests that connects C++ client and a mock C++ server to test authentication Closes apache#8724 from lyndonb-bq/jduo/lyndon/flight-auth-cpp-redesign-client Authored-by: Lyndon Bauto <lyndonb@bitquilltech.com> Signed-off-by: David Li <li.davidm96@gmail.com>

lyndonbauto added 8 commits November 18, 2020 15:22

[1] Initial commit to support client header authentication in C++

c2ba2c7

[1] Added integration test for client header authentication in C++ an…

ab7d6a3

…d server header authentication in Java

Merge branch 'lyndon/flight-auth-redesign-cpp' of https://github.com/…

ecb533f

…lyndonb-bq/arrow into jduo/lyndon/flight-auth-cpp-redesign-client

[1] Updates for linting

73be3e7

[1] Adding missed file.

74ef8ea

[1] Adding fix for Java lint errors.

29a3192

[1] Added a couple comments

fac0bd0

[1] Minor comment fixes

7fd1279

github-actions bot added Component: FlightRPC Component: C++ Component: Java labels Nov 20, 2020

lidavidm self-requested a review November 20, 2020 18:49

kylepbit reviewed Nov 20, 2020

View reviewed changes

lidavidm requested changes Nov 20, 2020

View reviewed changes

[1] Addressed pull request comments, still need to address comments a…

0b5a08e

…round unit tests but checking in WIP.

[1] Trying different cmake

ba7cb9f

lyndonbauto added 14 commits November 24, 2020 11:14

[1] Addressed code review comments.

1426252

[1] Removing integration test and reverting some cmake changes.

516d993

[1] Removed some no longer used functionality.

3000ecb

[2] Fixed option passing.

[1] Added improved testing and fixed linting.

1de10fa

[1] Fixed lint issue

065af4a

Merge branch 'master' of https://github.com/apache/arrow into jduo/ly…

d4da03b

…ndon/flight-auth-cpp-redesign-client

[1] Updating submodule

911fcc7

[1] Minor documentation fixes.

f41edce

[1] Fixed casting issue on some builds

6b6fbbe

[1] Added missing parameter for documentation

199b655

[1] Fixing cast.

47aa581

[1] Moving std:: from toupper call because it causes break in some bu…

477d865

…ilds.

[1] Adding missed std remove

1cc3fdb

[1] Fixed linting issue.

d27465d

lyndonbauto requested a review from lidavidm November 25, 2020 16:49

lidavidm reviewed Nov 25, 2020

View reviewed changes

lyndonbauto added 2 commits November 25, 2020 10:58

[1] Updated test return error properly and to check for error

d21006f

[2] Switched return type of AuthenticateBasicToken to use arrow::Result instead of Status and removed bearer token from parameter list

[1] Fixed linting issue.

6cd8a45

lyndonbauto requested a review from lidavidm November 25, 2020 20:06

lidavidm approved these changes Nov 25, 2020

View reviewed changes

lidavidm changed the title ~~ARROW-10487 [FlightRPC][C++][Python] Header-based auth in clients~~ ARROW-10487 [FlightRPC][C++] Header-based auth in clients Nov 25, 2020

lidavidm closed this in 65aa527 Nov 25, 2020

tifflhl mentioned this pull request Dec 21, 2020

ARROW-11004: [FlightRPC][Python] Header-based auth in clients #8959

Closed

asfimport mentioned this pull request Nov 25, 2020

[FlightRPC][C++] Header-based auth in clients #18355

Closed


		void ClientBearerTokenMiddleware::CallCompleted(const Status& status) { }

		void ClientBearerTokenFactory::StartCall(const CallInfo& info, std::unique_ptr<ClientMiddleware>* middleware) {

		@@ -0,0 +1,78 @@
		// Licensed to the Apache Software Foundation (ASF) under one

ARROW-10487 [FlightRPC][C++] Header-based auth in clients #8724

ARROW-10487 [FlightRPC][C++] Header-based auth in clients #8724

Conversation

lyndonbauto commented Nov 20, 2020 • edited Loading

github-actions bot commented Nov 20, 2020

lyndonbauto commented Nov 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lidavidm commented Nov 20, 2020 • edited Loading

Choose a reason for hiding this comment

lidavidm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lidavidm commented Nov 24, 2020

lidavidm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyndonbauto commented Nov 25, 2020

lyndonbauto commented Nov 25, 2020

lidavidm commented Nov 25, 2020

lyndonbauto commented Nov 20, 2020 •

edited

Loading

lidavidm commented Nov 20, 2020 •

edited

Loading