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

tifflhl · 2020-12-18T03:01:46Z

No description provided.

github-actions · 2020-12-18T03:01:57Z

Thanks for opening a pull request!

Could you open an issue for this pull request on JIRA?
https://issues.apache.org/jira/browse/ARROW

Then could you also rename pull request title in the following format?

ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}

See also:

jduo · 2020-12-18T06:28:23Z

python/pyarrow/tests/test_flight.py

+        return ""
+
+
+class HeaderAuthServerMiddlewareFactory(ServerMiddlewareFactory):


This class should not be part of test_flight.py to be used in production. It should also be changed to have some API for checking username/password/bearer rather than being hard coded.

Unless this PR is really for header-based auth in just clients, in which case the description is not really correct.

This PR is only for the client implementation of header based auth in Python, same as the C++ implementation done here: #8724. I've created a new subtask in ARROW-10486 that reflects this scope of work: https://issues.apache.org/jira/browse/ARROW-11004.

jduo · 2020-12-21T21:14:13Z

python/pyarrow/_flight.pyx

@@ -1150,6 +1156,38 @@ cdef class FlightClient(_Weakrefable):
                self.client.get().Authenticate(deref(c_options),
                                               move(handler)))

+    def authenticateBasicToken(self, username, password,
+                               options: FlightCallOptions = None):
+        """Authenticate to the server with header token authentication.


It's not really header token authentication, it's HTTP basic authentication

Addressed in new commit.

jduo · 2020-12-21T21:14:55Z

python/pyarrow/includes/libarrow_flight.pxd

@@ -307,6 +308,12 @@ cdef extern from "arrow/flight/api.h" namespace "arrow" nogil:
        CStatus Authenticate(CFlightCallOptions& options,
                             unique_ptr[CClientAuthHandler] auth_handler)

+        # TODO: Add AuthenticateBasicToken


Remove TODO

Addressed in new commit.

jduo · 2020-12-21T21:15:59Z

python/pyarrow/tests/test_flight.py

+
+    def is_valid(self, token):
+        """Do nothing."""
+        return ""


Better to return None than empty string?

Returning None gives the error Flight RPC failed with Python exception "TypeError: expected bytes, NoneType found". Based on the method's documentation here, returning an empty string is allowed, I added a comment to clarify the behaviour.

jduo · 2020-12-21T21:17:13Z

python/pyarrow/tests/test_flight.py

+    """Validates incoming username and password."""
+
+    def start_call(self, info, headers):
+        auth_header = headers.get('authorization')


This is test code, but this should be a case insensitive lookup.

Addressed in new commit.

jduo · 2020-12-21T21:18:48Z

python/pyarrow/tests/test_flight.py

+        self.token = token
+
+    def sending_headers(self):
+        return {'authorization': 'Bearer ' + self.token}


We had been using Authorization (title case) in other languages.

It would seem the C++ grpc impl does not allow capitals in key, when using capital A, the header was rejected due to https://github.com/grpc/grpc/blob/b1df40104c1720bde3b22ded451a037f11b5dc54/src/core/lib/surface/validate_metadata.cc#L78

Should that be a comment for the headers parameter?

Headers are supposed to be treated case-insensitively so even though other languages may use Authorization, it will all get folded to the same case

jduo · 2020-12-21T21:20:09Z

python/pyarrow/tests/test_flight.py

+            auth_header = headers.get('authorization')
+            values = auth_header.split(' ')
+            return [values[1].encode("utf-8")]
+        raise flight.FlightUnauthenticatedError(


This seems like it's an internal server error.

Addressed in new commit.

jduo · 2020-12-21T21:25:16Z

python/pyarrow/_flight.pyx

+            c_string pw = tobytes(password)
+
+        with nogil:
+            result = self.client.get().AuthenticateBasicToken(deref(c_options),


Shouldn't there be a FlightCallOption implementation to do this? How do you use HTTP basic auth without using authenticateBasicToken? (eg in Java you can directly call getFlightInfo without calling an authenticate function and supply a FlightCallOption with the basic auth info).

Based on how the C++ code was implemented , the Python wrapper can only wrap the AuthenticateBasicToken method in the C++ client. Seems like we would need to expand the C++ client implementation if we were to have C++/Python impl match the Java client implementation. #8724 -> C++ impl only exposed authenticateBasicToken.

@jduo are the changes in this PR sufficient?

github-actions · 2020-12-21T22:11:07Z

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

tifflhl · 2020-12-22T00:46:29Z

I added a test example implementation called ClientHeaderAuthMiddleware, it intercepts headers from a HTTP header auth enabled server and make it accessible to the code using the FlightClient. A new test also demonstrates how to use the ClientMiddleware to intercept an Authorization header returned from the server and to use it in subsequent calls.
I don't think it makes sense to include the example ClientMiddleware to the .pyx pr .pxd, since they are not callable objects in C++. Also seems like a convention to not define explicit middleware objects in python, but rather, provide examples?

stevelorddremio · 2020-12-23T16:50:56Z

python/pyarrow/_flight.pyx

@@ -1150,6 +1156,38 @@ cdef class FlightClient(_Weakrefable):
                self.client.get().Authenticate(deref(c_options),
                                               move(handler)))

+    def authenticateBasicToken(self, username, password,


Do you need to add a description for self in the list of parameters below?

self is never documented - it's implicitly passed by Python (it's equivalent to this in Java et al)

stevelorddremio · 2020-12-23T16:54:09Z

python/pyarrow/_flight.pyx

@@ -1871,7 +1909,6 @@ cdef CStatus _server_authenticate(void* self, CServerAuthSender* outgoing,
        reader.poison()


Is this a typo "poison"?

Not a typo.

This is to ensure a server doesn't use the Python reader beyond the lifetime of the C++ reader it wraps.

stevelorddremio · 2020-12-23T17:02:58Z

python/pyarrow/includes/libarrow_flight.pxd

@@ -307,6 +308,11 @@ cdef extern from "arrow/flight/api.h" namespace "arrow" nogil:
        CStatus Authenticate(CFlightCallOptions& options,
                             unique_ptr[CClientAuthHandler] auth_handler)

+        CResult[pair[c_string, c_string]] AuthenticateBasicToken(
+            CFlightCallOptions& options,
+            const c_string& username,


For the uninitiated, what are the coding standards on Class & variable names? I see a mixture of camel case and underscore separated names.

The mixture here is because this is a set of Cython type definitions for C++ code, which uses the C++ conventions (e.g. CResult), but the overall project is in Python, which uses Python conventions (e.g. c_string)

stevelorddremio · 2020-12-23T17:05:05Z

python/pyarrow/tests/test_flight.py

+        self.factory = factory
+
+    def received_headers(self, headers):
+        auth_header = case_insensitive_header_lookup(headers, 'Authorization')


There seems to be a mix of single and double quotes for strings. Should these be consistent?

The two are interchangeable.

stevelorddremio · 2020-12-23T17:10:20Z

python/pyarrow/tests/test_flight.py

+            decoded = base64.b64decode(values[1])
+            pair = decoded.decode("utf-8").split(':')
+            if not (pair[0] == 'test' and pair[1] == 'password'):
+                raise flight.FlightUnauthenticatedError('Invalid credentials')


3 uses of the string 'Invalid credentials'. Should this be made a constant?

lidavidm · 2020-12-23T20:06:46Z

python/pyarrow/_flight.pyx

@@ -1150,6 +1156,38 @@ cdef class FlightClient(_Weakrefable):
                self.client.get().Authenticate(deref(c_options),
                                               move(handler)))

+    def authenticateBasicToken(self, username, password,


Let's follow Python naming conventions - this should use snake_case.

lidavidm · 2020-12-23T20:07:20Z

python/pyarrow/_flight.pyx

@@ -118,12 +118,18 @@ cdef class FlightCallOptions(_Weakrefable):
        write_options : pyarrow.ipc.IpcWriteOptions, optional
            IPC write options. The default options can be controlled
            by environment variables (see pyarrow.ipc).
-
+        headers : vector[pair[c_string, c_string]], optional


This type hint should use Python conventions (e.g. List[Tuple[str, str]])

lidavidm · 2020-12-23T20:08:23Z

python/pyarrow/_flight.pyx

+
+        Returns
+        -------
+        pair : pair[string, string]


Same for the type hints here - use str and Tuple

lidavidm · 2020-12-23T20:09:50Z

python/pyarrow/includes/libarrow_flight.pxd

@@ -307,6 +308,11 @@ cdef extern from "arrow/flight/api.h" namespace "arrow" nogil:
        CStatus Authenticate(CFlightCallOptions& options,
                             unique_ptr[CClientAuthHandler] auth_handler)

+        CResult[pair[c_string, c_string]] AuthenticateBasicToken(
+            CFlightCallOptions& options,
+            const c_string& username,


The mixture here is because this is a set of Cython type definitions for C++ code, which uses the C++ conventions (e.g. CResult), but the overall project is in Python, which uses Python conventions (e.g. c_string)

lidavidm · 2020-12-23T20:11:05Z

python/pyarrow/tests/test_flight.py

+        return ""
+
+
+def case_insensitive_header_lookup(headers, lookup_key):


This is specifically to extract an authentication header based n the exception it raises, let's make sure the name reflects that.

I modified the function to not raise an exception when the header is not found. The method is used in a test not related to authentication as well.

lidavidm · 2020-12-23T20:12:22Z

python/pyarrow/tests/test_flight.py

+        self.token = token
+
+    def sending_headers(self):
+        return {'authorization': 'Bearer ' + self.token}


Headers are supposed to be treated case-insensitively so even though other languages may use Authorization, it will all get folded to the same case

lidavidm · 2020-12-23T20:24:16Z

python/pyarrow/tests/test_flight.py

+        client = FlightClient(('localhost', server.port))
+        token_pair = client.authenticateBasicToken(b'test', b'password')
+        assert token_pair[0] == b'authorization'
+        assert token_pair[1] == b'Bearer ' + b'token1234'


nit: why concat here? (and above)

lidavidm · 2020-12-23T20:26:05Z

python/pyarrow/_flight.pyx

@@ -1871,7 +1909,6 @@ cdef CStatus _server_authenticate(void* self, CServerAuthSender* outgoing,
        reader.poison()


Not a typo.

lidavidm · 2020-12-23T20:26:43Z

python/pyarrow/_flight.pyx

@@ -1871,7 +1909,6 @@ cdef CStatus _server_authenticate(void* self, CServerAuthSender* outgoing,
        reader.poison()


This is to ensure a server doesn't use the Python reader beyond the lifetime of the C++ reader it wraps.

lidavidm · 2020-12-23T20:27:18Z

python/pyarrow/_flight.pyx

@@ -1150,6 +1156,38 @@ cdef class FlightClient(_Weakrefable):
                self.client.get().Authenticate(deref(c_options),
                                               move(handler)))

+    def authenticateBasicToken(self, username, password,


self is never documented - it's implicitly passed by Python (it's equivalent to this in Java et al)

lidavidm · 2020-12-26T16:49:19Z

python/pyarrow/_flight.pyx

+            c_string pw = tobytes(password)
+
+        with nogil:
+            result = self.client.get().AuthenticateBasicToken(deref(c_options),


@jduo are the changes in this PR sufficient?

jduo · 2020-12-26T19:51:42Z

@lidavidm yes these changes are sufficient.

Closes apache#8959 from tifflhl/ARROW-10486 Authored-by: tifflhl <tiffanylamhl@gmail.com> Signed-off-by: David Li <li.davidm96@gmail.com>

tifflhl added 8 commits December 13, 2020 19:44

ARROW-10486: [Python][Flight] Header-based authentication

d241e50

Improve tests

206e1c8

Comment out tests for debugging

c0dcca2

Use nogil with authenticateBasicToken call

661a2a3

Fix unchecked error messages

b999123

Remove explicit constructors

c162932

Lint files

2cf5bbf

Fix remaining integration tests error

35e79e8

tifflhl changed the title ~~Arrow 10486: [Python] Header-based auth in servers~~ Arrow-10486: [Python] Header-based auth in servers Dec 18, 2020

github-actions bot added Component: FlightRPC Component: Python labels Dec 18, 2020

tifflhl changed the title ~~Arrow-10486: [Python] Header-based auth in servers~~ Arrow-10486: [Python] Header-based auth in client Dec 18, 2020

jduo requested changes Dec 21, 2020

View reviewed changes

tifflhl changed the title ~~Arrow-10486: [Python] Header-based auth in client~~ ARROW-10486: [Python] Header-based auth in client Dec 21, 2020

tifflhl changed the title ~~ARROW-10486: [Python] Header-based auth in client~~ ARROW-11004: [Python] Header-based auth in client Dec 21, 2020

tifflhl changed the title ~~ARROW-11004: [Python] Header-based auth in client~~ ARROW-11004: [FlightRPC][Python] Header-based auth in clients Dec 21, 2020

tifflhl added 3 commits December 21, 2020 15:43

Address code review comments

87043dd

Add test for authentication interception with do_action call

ce33893

Fix typo and enhanced unit tests

a861ac1

Lint files

638113a

jduo approved these changes Dec 22, 2020

View reviewed changes

stevelorddremio reviewed Dec 23, 2020

View reviewed changes

lidavidm self-requested a review December 23, 2020 19:49

lidavidm requested changes Dec 23, 2020

View reviewed changes

tifflhl added 2 commits December 23, 2020 20:07

Address code review comments

7c06d01

Lint files

0ed06a9

lidavidm approved these changes Dec 26, 2020

View reviewed changes

lidavidm closed this in 4d97d83 Dec 26, 2020

asfimport mentioned this pull request Dec 26, 2020

[FlightRPC][Python] Header-based auth in clients #26924

Closed

		return ""


		class HeaderAuthServerMiddlewareFactory(ServerMiddlewareFactory):

		@@ -1871,7 +1909,6 @@ cdef CStatus _server_authenticate(void* self, CServerAuthSender* outgoing,
		reader.poison()

		return ""


		def case_insensitive_header_lookup(headers, lookup_key):

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

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

Conversation

tifflhl commented Dec 18, 2020

github-actions bot commented Dec 18, 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

github-actions bot commented Dec 21, 2020

tifflhl commented Dec 22, 2020

stevelorddremio Dec 23, 2020 • edited Loading

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

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

jduo commented Dec 26, 2020

stevelorddremio Dec 23, 2020 •

edited

Loading