You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@arrow.apache.org by "kou (via GitHub)" <gi...@apache.org> on 2023/04/30 07:12:54 UTC
[GitHub] [arrow] kou opened a new issue, #35373: [C++][FlightRPC] Add support for using HTTP headers or query parameters in ServerAuthHandler
kou opened a new issue, #35373:
URL: https://github.com/apache/arrow/issues/35373
### Describe the enhancement requested
`arrow::flight::ServerAuthHandler::Authenticate()` can't refer HTTP headers nor HTTP query parameters:
https://github.com/apache/arrow/blob/0ea1a103dfc2fc4ab5a5d839369a805e2ae657a2/cpp/src/arrow/flight/server_auth.h#L56-L58
I want to receive database name via a HTTP header or a HTTP query parameter in https://github.com/apache/arrow-flight-sql-postgresql .
How about adding `const CallHeaders& incoming_headers` parameter (like for `arrow::flight::ServerMiddlewareFactory::StartCall()`) or something to `arrow::flight::ServerAuthHandler::Authenticate()`?
@lidavidm @cyb70289 What do you think about this feature request?
### Component(s)
C++, FlightRPC
--
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.
To unsubscribe, e-mail: issues-unsubscribe@arrow.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou closed issue #35373: [C++][FlightRPC] Add support for using HTTP headers or query parameters in ServerAuthHandler
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou closed issue #35373: [C++][FlightRPC] Add support for using HTTP headers or query parameters in ServerAuthHandler
URL: https://github.com/apache/arrow/issues/35373
--
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.
To unsubscribe, e-mail: issues-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on issue #35373: [C++][FlightRPC] Add support for using HTTP headers or query parameters in ServerAuthHandler
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on issue #35373:
URL: https://github.com/apache/arrow/issues/35373#issuecomment-1529271268
> Or adding `const ServerCallContext& context` argument?
Ah to be clear: I think this would also be a good idea regardless.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on issue #35373: [C++][FlightRPC] Add support for using HTTP headers or query parameters in ServerAuthHandler
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on issue #35373:
URL: https://github.com/apache/arrow/issues/35373#issuecomment-1529330080
I think we should keep compatibility for a few releases. The redundancy is unfortunate but we can't avoid it.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on issue #35373: [C++][FlightRPC] Add support for using HTTP headers or query parameters in ServerAuthHandler
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on issue #35373:
URL: https://github.com/apache/arrow/issues/35373#issuecomment-1529270134
> Is this basically the same idea as #34817?
Ah, sorry. I haven't read the discussion on the PR.
> I'm reluctant to expand Handshake because it's stateful. I think the explicit messages in that PR are better, along with support for just passing the value via headers always.
OK. I withdraw this idea.
> > But we also need to add `ServerCallContext::incoming_headers()` for this idea.
>
> I think we should do this anyways.
OK. I've opened #35375.
> > I want to receive database name via a HTTP header or a HTTP query parameter in https://github.com/apache/arrow-flight-sql-postgresql .
>
> I don't think query parameters get passed through via gRPC, unfortunately. So only headers would work.
Oh, sorry. I should have learned more about gRPC...
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on issue #35373: [C++][FlightRPC] Add support for using HTTP headers or query parameters in ServerAuthHandler
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on issue #35373:
URL: https://github.com/apache/arrow/issues/35373#issuecomment-1529314906
Ah, OK. I'll open a new issue for it.
Should we keep a backward compatibility for it?
If we should keep a backward compatibility, we will use the following changes:
```diff
diff --git a/cpp/src/arrow/flight/server_auth.h b/cpp/src/arrow/flight/server_auth.h
index b1ccb096d..2f63d0b23 100644
--- a/cpp/src/arrow/flight/server_auth.h
+++ b/cpp/src/arrow/flight/server_auth.h
@@ -55,6 +55,11 @@ class ARROW_FLIGHT_EXPORT ServerAuthHandler {
virtual ~ServerAuthHandler();
/// \brief Authenticate the client on initial connection. The server
/// can send and read responses from the client at any time.
+ virtual Status Authenticate(const ServerCallContext& context, ServerAuthSender* outgoing, ServerAuthReader* incoming) {
+ return Authenticate(outgoing, incoming);
+ }
+ /// \brief Authenticate the client on initial connection. The server
+ /// can send and read responses from the client at any time.
virtual Status Authenticate(ServerAuthSender* outgoing, ServerAuthReader* incoming) = 0;
/// \brief Validate a per-call client token.
/// \param[in] token The client token. May be the empty string if
@@ -63,6 +68,15 @@ class ARROW_FLIGHT_EXPORT ServerAuthHandler {
/// authentication method supports it.
/// \return Status OK if the token is valid, any other status if
/// validation failed
+ virtual Status IsValid(const ServerCallContext& context, const std::string& token, std::string* peer_identity) {
+ return IsValid(outgoing, incoming);
+ }
+ /// \param[in] token The client token. May be the empty string if
+ /// the client does not provide a token.
+ /// \param[out] peer_identity The identity of the peer, if this
+ /// authentication method supports it.
+ /// \return Status OK if the token is valid, any other status if
+ /// validation failed
virtual Status IsValid(const std::string& token, std::string* peer_identity) = 0;
};
```
If we should not keep a backward compatibility, we will use the following changes:
```diff
diff --git a/cpp/src/arrow/flight/server_auth.h b/cpp/src/arrow/flight/server_auth.h
index b1ccb096d..391910548 100644
--- a/cpp/src/arrow/flight/server_auth.h
+++ b/cpp/src/arrow/flight/server_auth.h
@@ -55,7 +55,7 @@ class ARROW_FLIGHT_EXPORT ServerAuthHandler {
virtual ~ServerAuthHandler();
/// \brief Authenticate the client on initial connection. The server
/// can send and read responses from the client at any time.
- virtual Status Authenticate(ServerAuthSender* outgoing, ServerAuthReader* incoming) = 0;
+ virtual Status Authenticate(const ServerCallContext& context, ServerAuthSender* outgoing, ServerAuthReader* incoming) = 0;
/// \brief Validate a per-call client token.
/// \param[in] token The client token. May be the empty string if
/// the client does not provide a token.
@@ -63,7 +63,7 @@ class ARROW_FLIGHT_EXPORT ServerAuthHandler {
/// authentication method supports it.
/// \return Status OK if the token is valid, any other status if
/// validation failed
- virtual Status IsValid(const std::string& token, std::string* peer_identity) = 0;
+ virtual Status IsValid(const ServerCallContext& context, const std::string& token, std::string* peer_identity) = 0;
};
/// \brief An authentication mechanism that does nothing.
```
I think that we should keep backward compatibility, but user code will be redundant. Because user code needs to implement `Status Authenticate(ServerAuthSender* outgoing, ServerAuthReader* incoming)` too:
```cpp
Status Authenticate(const ServerCallContext& context, ServerAuthSender* outgoing, ServerAuthReader* incoming) {
// Implement...
}
Status Authenticate(ServerAuthSender* outgoing, ServerAuthReader* incoming) {
return Status::NotImplemented("This version is never 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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on issue #35373: [C++][FlightRPC] Add support for using HTTP headers or query parameters in ServerAuthHandler
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on issue #35373:
URL: https://github.com/apache/arrow/issues/35373#issuecomment-1528979791
Is this basically the same idea as #34817?
I'm reluctant to expand Handshake because it's stateful. I think the explicit messages in that PR are better, along with support for just passing the value via headers always.
That said, if we are to expand this method I think it should be via adding `const ServerCallContext& context` because technically you are in an RPC call during execution.
> But we also need to add `ServerCallContext::incoming_headers()` for this idea.
I think we should do this anyways.
> I want to receive database name via a HTTP header or a HTTP query parameter in https://github.com/apache/arrow-flight-sql-postgresql .
I don't think query parameters get passed through via gRPC, unfortunately. So only headers would 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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on issue #35373: [C++][FlightRPC] Add support for using HTTP headers or query parameters in ServerAuthHandler
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on issue #35373:
URL: https://github.com/apache/arrow/issues/35373#issuecomment-1529394030
I agree with you. I've opened #35377.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on issue #35373: [C++][FlightRPC] Add support for using HTTP headers or query parameters in ServerAuthHandler
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on issue #35373:
URL: https://github.com/apache/arrow/issues/35373#issuecomment-1528958343
Or adding `const ServerCallContext& context` argument?
But we also need to add `ServerCallContext::incoming_headers()` for this idea.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org