Substitute credentials authentication by token authentication with auto-renewal #743
Conversation
PR Review ChecklistDo not edit the content of this comment. The PR reviewer should simply update this comment by ticking each review item below, as they get completed. Trivial Change
Code
Architecture
|
| @@ -275,6 +275,16 @@ impl Error { | |||
| } | |||
| } | |||
|
|
|||
| fn try_extracting_connection_error(status: &Status, code: &str) -> Option<ConnectionError> { | |||
| // TODO: We should probably catch more connection errors instead of wrapping them into | |||
There was a problem hiding this comment.
I don't really want to refactor error messaging here.
| if let Some(token) = &*self.token.read().expect("Expected token read lock acquisition on inject") { | ||
| request.metadata_mut().insert( | ||
| "authorization", | ||
| format!("Bearer {}", token).try_into().expect("Expected authorization header formatting"), |
There was a problem hiding this comment.
It's more a question about the server, but I decided to follow the standard HTTP format of authorization: Bearer <TOKEN> metadata records. It requires manual parsing in tonic and is done better in axum (for http), but I haven't found any explicit recommendation to turn away from the HTTP standard in gRPC and drop the Bearer part. Although we used to just set token: ... in 2.x.
There was a problem hiding this comment.
I don't have a strong knowledge/opinion on this, maybe @lolski has some?
| @@ -45,15 +48,41 @@ impl<Channel: GRPCChannel> RPCStub<Channel> { | |||
| Self { grpc: GRPC::new(channel), call_credentials } | |||
| } | |||
|
|
|||
| async fn call<F, R>(&mut self, call: F) -> Result<R> | |||
| async fn call_with_auto_renew_token<F, R>(&mut self, call: F) -> Result<R> | |||
There was a problem hiding this comment.
This part is actually a copypaste from 2.x.
| where | ||
| for<'a> F: Fn(&'a mut Self) -> BoxFuture<'a, Result<R>>, | ||
| { | ||
| call(self).await | ||
| match call(self).await { | ||
| Err(Error::Connection(ConnectionError::TokenCredentialInvalid)) => { |
There was a problem hiding this comment.
If it failed specifically, renew the token and retry.
| } | ||
|
|
||
| pub(super) async fn connection_open(&mut self, req: connection::open::Req) -> Result<connection::open::Res> { | ||
| self.single(|this| Box::pin(this.grpc.connection_open(req.clone()))).await | ||
| let result = self.single(|this| Box::pin(this.grpc.connection_open(req.clone()))).await; |
There was a problem hiding this comment.
Sneak into the response to get the token on the network level without asking the more business-like logic to work with it for us.
| @@ -22,6 +22,7 @@ rm -rf typedb-all | |||
|
|
|||
| bazel run //tool/test:typedb-extractor -- typedb-all | |||
| BAZEL_JAVA_HOME=$(bazel run //tool/test:echo-java-home) | |||
| # TODO: Can add `--server.authentication.token_ttl_seconds 15` to test token auto-renewal in BDDs! | |||
There was a problem hiding this comment.
I don't have the artifacts (I just tested locally), so this feature can be good. We have small integration tests, some small behaviour tests, and some big behaviour tests. Everything will be covered!
|
It's expected to be red as I don't have the new server artifacts. It's green locally. |
| // ServerErrors. However, the most valuable information even for connection is inside | ||
| // stacktraces now. | ||
| match code { | ||
| "AUT3" => Some(ConnectionError::TokenCredentialInvalid {}), |
There was a problem hiding this comment.
Ouu ok make a comment in the server-side that we depend on those error messages client-side and not to change them randomly -- if we don't already have that warning!
There was a problem hiding this comment.
We did a similar thing (even for a bigger number of errors) in 2.x in other domains. But I will. And the BDDs with server running with shorter tokens will show if we don't actually renew them.
Usage and product changes
Upgrade TypeDB protocol to use authentication tokens instead of passing credentials with every API call.
When sending gRPC requests, drivers include temporary tokens in the request headers, limiting password usage to
connection_openand token renewal calls, providing better security for sensitive data.The authentication and token renewal mechanisms are fully automatic and do not require any actions from TypeDB driver users. Thus, the API remains unchanged.
Implementation
Credential injectors start injecting tokens instead of credentials.
These tokens are acquired when:
connection_open);sign_incall. This operation is expected to be insignificant in terms of performance effect since the users are not expected to set token TTLs to too low values.