Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
tifflhl wants to merge 14 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
d241e50
ARROW-10486: [Python][Flight] Header-based authentication
tifflhl Dec 14, 2020
206e1c8
Improve tests
tifflhl Dec 14, 2020
c0dcca2
Comment out tests for debugging
tifflhl Dec 14, 2020
661a2a3
Use nogil with authenticateBasicToken call
tifflhl Dec 16, 2020
b999123
Fix unchecked error messages
tifflhl Dec 17, 2020
c162932
Remove explicit constructors
tifflhl Dec 17, 2020
2cf5bbf
Lint files
tifflhl Dec 18, 2020
35e79e8
Fix remaining integration tests error
tifflhl Dec 18, 2020
87043dd
Address code review comments
tifflhl Dec 21, 2020
ce33893
Add test for authentication interception with do_action call
tifflhl Dec 22, 2020
a861ac1
Fix typo and enhanced unit tests
tifflhl Dec 22, 2020
638113a
Lint files
tifflhl Dec 22, 2020
7c06d01
Address code review comments
tifflhl Dec 24, 2020
0ed06a9
Lint files
tifflhl Dec 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 42 additions & 5 deletions python/pyarrow/_flight.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ cdef class FlightCallOptions(_Weakrefable):
cdef:
CFlightCallOptions options

def __init__(self, timeout=None, write_options=None):
def __init__(self, timeout=None, write_options=None, headers=None):
"""Create call options.

Parameters
Expand All @@ -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 : List[Tuple[str, str]], optional
A list of arbitrary headers as key, value tuples
"""
cdef IpcWriteOptions options = _get_options(write_options)
cdef IpcWriteOptions c_write_options

if timeout is not None:
self.options.timeout = CTimeoutDuration(timeout)
self.options.write_options = options.c_options
if write_options is not None:
c_write_options = _get_options(write_options)
self.options.write_options = c_write_options.c_options
if headers is not None:
self.options.headers = headers

@staticmethod
cdef CFlightCallOptions* unwrap(obj):
Expand Down Expand Up @@ -1150,6 +1156,38 @@ cdef class FlightClient(_Weakrefable):
self.client.get().Authenticate(deref(c_options),
move(handler)))

def authenticate_basic_token(self, username, password,
options: FlightCallOptions = None):
"""Authenticate to the server with HTTP basic authentication.

Parameters
----------
username : string
Username to authenticate with
password : string
Password to authenticate with
options : FlightCallOptions
Options for this call

Returns
-------
tuple : Tuple[str, str]
A tuple representing the FlightCallOptions authorization
header entry of a bearer token.
"""
cdef:
CResult[pair[c_string, c_string]] result
CFlightCallOptions* c_options = FlightCallOptions.unwrap(options)
c_string user = tobytes(username)
c_string pw = tobytes(password)

with nogil:
result = self.client.get().AuthenticateBasicToken(deref(c_options),
Copy link
Member

@jduo jduo Dec 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

@tifflhl tifflhl Dec 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@lidavidm lidavidm Dec 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jduo are the changes in this PR sufficient?

user, pw)
check_flight_status(result.status())

return GetResultValue(result)

def list_actions(self, options: FlightCallOptions = None):
"""List the actions available on a service."""
cdef:
Expand Down Expand Up @@ -1871,7 +1909,6 @@ cdef CStatus _server_authenticate(void* self, CServerAuthSender* outgoing,
reader.poison()
Copy link
Contributor

@stevelorddremio stevelorddremio Dec 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a typo "poison"?

Copy link
Member

@lidavidm lidavidm Dec 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a typo.

Copy link
Member

@lidavidm lidavidm Dec 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return CStatus_OK()


cdef CStatus _is_valid(void* self, const c_string& token,
c_string* peer_identity) except *:
"""Callback for implementing authentication in Python."""
Expand Down
6 changes: 6 additions & 0 deletions python/pyarrow/includes/libarrow_flight.pxd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,7 @@ cdef extern from "arrow/flight/api.h" namespace "arrow" nogil:
CFlightCallOptions()
CTimeoutDuration timeout
CIpcWriteOptions write_options
vector[pair[c_string, c_string]] headers

cdef cppclass CCertKeyPair" arrow::flight::CertKeyPair":
CCertKeyPair()
Expand Down Expand Up @@ -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,
Copy link
Contributor

@stevelorddremio stevelorddremio Dec 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@lidavidm lidavidm Dec 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

const c_string& password)

CStatus DoAction(CFlightCallOptions& options, CAction& action,
unique_ptr[CResultStream]* results)
CStatus ListActions(CFlightCallOptions& options,
Expand Down
Loading