-
Notifications
You must be signed in to change notification settings - Fork 34
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.
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
Substitute credentials authentication by token authentication with auto-renewal #743
base: 3.0
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,7 +21,7 @@ use std::{collections::HashSet, error::Error as StdError, fmt}; | |
|
||
use itertools::Itertools; | ||
use tonic::{Code, Status}; | ||
use tonic_types::StatusExt; | ||
use tonic_types::{ErrorDetails, ErrorInfo, StatusExt}; | ||
|
||
use super::{address::Address, RequestID}; | ||
|
||
|
@@ -150,7 +150,7 @@ error_messages! { ConnectionError | |
15: "The replica is not the primary replica.", | ||
ClusterAllNodesFailed { errors: String } = | ||
16: "Attempted connecting to all TypeDB Cluster servers, but the following errors occurred: \n{errors}.", | ||
ClusterTokenCredentialInvalid = | ||
TokenCredentialInvalid = | ||
17: "Invalid token credentials.", | ||
EncryptionSettingsMismatch = | ||
18: "Unable to connect to TypeDB: possible encryption settings mismatch.", | ||
|
@@ -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 | ||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
_ => None, | ||
} | ||
} | ||
|
||
fn from_message(message: &str) -> Self { | ||
// TODO: Consider converting some of the messages to connection errors | ||
Self::Other(message.to_owned()) | ||
|
@@ -352,9 +362,13 @@ impl From<Status> for Error { | |
}) | ||
} else if let Some(error_info) = details.error_info() { | ||
let code = error_info.reason.clone(); | ||
if let Some(connection_error) = Self::try_extracting_connection_error(&status, &code) { | ||
return Self::Connection(connection_error); | ||
} | ||
let domain = error_info.domain.clone(); | ||
let stack_trace = | ||
if let Some(debug_info) = details.debug_info() { debug_info.stack_entries.clone() } else { vec![] }; | ||
|
||
Self::Server(ServerError::new(code, domain, status.message().to_owned(), stack_trace)) | ||
} else { | ||
Self::from_message(status.message()) | ||
|
@@ -364,7 +378,6 @@ impl From<Status> for Error { | |
Self::parse_unavailable(status.message()) | ||
} else if status.code() == Code::Unknown | ||
|| is_rst_stream(&status) | ||
|| status.code() == Code::InvalidArgument | ||
|| status.code() == Code::FailedPrecondition | ||
|| status.code() == Code::AlreadyExists | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,11 +17,12 @@ | |
* under the License. | ||
*/ | ||
|
||
use std::sync::Arc; | ||
use std::sync::{Arc, RwLock}; | ||
|
||
use tonic::{ | ||
body::BoxBody, | ||
client::GrpcService, | ||
metadata::MetadataValue, | ||
service::{ | ||
interceptor::{InterceptedService, ResponseFuture as InterceptorResponseFuture}, | ||
Interceptor, | ||
|
@@ -65,20 +66,33 @@ pub(super) fn open_callcred_channel( | |
#[derive(Debug)] | ||
pub(super) struct CallCredentials { | ||
credentials: Credentials, | ||
token: RwLock<Option<String>>, | ||
} | ||
|
||
impl CallCredentials { | ||
pub(super) fn new(credentials: Credentials) -> Self { | ||
Self { credentials } | ||
Self { credentials, token: RwLock::new(None) } | ||
} | ||
|
||
pub(super) fn username(&self) -> &str { | ||
self.credentials.username() | ||
pub(super) fn credentials(&self) -> &Credentials { | ||
&self.credentials | ||
} | ||
|
||
pub(super) fn set_token(&self, token: String) { | ||
*self.token.write().expect("Expected token write lock acquisition on set") = Some(token); | ||
} | ||
|
||
pub(super) fn reset_token(&self) { | ||
*self.token.write().expect("Expected token write lock acquisition on reset") = None; | ||
} | ||
|
||
pub(super) fn inject(&self, mut request: Request<()>) -> Request<()> { | ||
request.metadata_mut().insert("username", self.credentials.username().try_into().unwrap()); | ||
request.metadata_mut().insert("password", self.credentials.password().try_into().unwrap()); | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's more a question about the server, but I decided to follow the standard HTTP format of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't have a strong knowledge/opinion on this, maybe @lolski has some? |
||
); | ||
} | ||
request | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,12 +25,15 @@ use tokio::sync::mpsc::{unbounded_channel as unbounded_async, UnboundedSender}; | |
use tokio_stream::wrappers::UnboundedReceiverStream; | ||
use tonic::{Response, Status, Streaming}; | ||
use typedb_protocol::{ | ||
connection, database, database_manager, server_manager, transaction, type_db_client::TypeDbClient as GRPC, user, | ||
user_manager, | ||
authentication, connection, database, database_manager, server_manager, transaction, | ||
type_db_client::TypeDbClient as GRPC, user, user_manager, | ||
}; | ||
|
||
use super::channel::{CallCredentials, GRPCChannel}; | ||
use crate::common::{error::ConnectionError, Error, Result, StdResult}; | ||
use crate::{ | ||
common::{error::ConnectionError, Error, Result, StdResult}, | ||
connection::network::proto::TryIntoProto, | ||
}; | ||
|
||
type TonicResult<T> = StdResult<Response<T>, Status>; | ||
|
||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it failed specifically, renew the token and retry. |
||
debug!("Request rejected because token credential was invalid. Renewing token and trying again..."); | ||
self.renew_token().await?; | ||
call(self).await | ||
} | ||
res => res, | ||
} | ||
} | ||
|
||
async fn renew_token(&mut self) -> Result { | ||
if let Some(call_credentials) = &self.call_credentials { | ||
trace!("Renewing token..."); | ||
call_credentials.reset_token(); | ||
let request = call_credentials.credentials().clone().try_into_proto()?; | ||
let token = self.grpc.sign_in(request).await?.into_inner().token; | ||
call_credentials.set_token(token); | ||
trace!("Token renewed"); | ||
} | ||
Ok(()) | ||
} | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sneak into the response to get the token on the |
||
if let Ok(response) = &result { | ||
if let Some(call_credentials) = &self.call_credentials { | ||
call_credentials | ||
.set_token(response.authentication.as_ref().expect("Expected authentication token").token.clone()); | ||
} | ||
} | ||
result | ||
} | ||
|
||
pub(super) async fn servers_all(&mut self, req: server_manager::all::Req) -> Result<server_manager::all::Res> { | ||
|
@@ -107,7 +136,7 @@ impl<Channel: GRPCChannel> RPCStub<Channel> { | |
&mut self, | ||
open_req: transaction::Req, | ||
) -> Result<(UnboundedSender<transaction::Client>, Streaming<transaction::Server>)> { | ||
self.call(|this| { | ||
self.call_with_auto_renew_token(|this| { | ||
let transaction_req = transaction::Client { reqs: vec![open_req.clone()] }; | ||
Box::pin(async { | ||
let (sender, receiver) = unbounded_async(); | ||
|
@@ -154,6 +183,6 @@ impl<Channel: GRPCChannel> RPCStub<Channel> { | |
for<'a> F: Fn(&'a mut Self) -> BoxFuture<'a, TonicResult<R>> + Send + Sync, | ||
R: 'static, | ||
{ | ||
self.call(|this| Box::pin(call(this).map(|r| Ok(r?.into_inner())))).await | ||
self.call_with_auto_renew_token(|this| Box::pin(call(this).map(|r| Ok(r?.into_inner())))).await | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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! |
||
./typedb-all/typedb server --development-mode.enabled & | ||
|
||
set +e | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really want to refactor error messaging here.