Skip to content

RUST-1663: Implement and enable reauthentication for OIDC #1044

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

Merged
merged 8 commits into from
Mar 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
38 changes: 38 additions & 0 deletions src/client/auth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,44 @@ impl AuthMechanism {
.into()),
}
}

pub(crate) async fn reauthenticate_stream(
&self,
stream: &mut Connection,
credential: &Credential,
server_api: Option<&ServerApi>,
) -> Result<()> {
self.validate_credential(credential)?;

match self {
AuthMechanism::ScramSha1
| AuthMechanism::ScramSha256
| AuthMechanism::MongoDbX509
| AuthMechanism::Plain
| AuthMechanism::MongoDbCr => Err(ErrorKind::Authentication {
message: format!(
"Reauthentication for authentication mechanism {:?} is not supported.",
self
),
}
.into()),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't seem that cfg is allowed within a pattern, unfortunately.

#[cfg(feature = "aws-auth")]
AuthMechanism::MongoDbAws => Err(ErrorKind::Authentication {
message: format!(
"Reauthentication for authentication mechanism {:?} is not supported.",
self
),
}
.into()),
AuthMechanism::MongoDbOidc => {
oidc::reauthenticate_stream(stream, credential, server_api).await
}
_ => Err(ErrorKind::Authentication {
message: format!("Authentication mechanism {:?} not yet implemented.", self),
}
.into()),
}
}
}

impl FromStr for AuthMechanism {
Expand Down
9 changes: 9 additions & 0 deletions src/client/auth/oidc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,15 @@ fn get_refresh_token_and_idp_info(
(refresh_token, idp_info)
}

pub(crate) async fn reauthenticate_stream(
conn: &mut Connection,
credential: &Credential,
server_api: Option<&ServerApi>,
) -> Result<()> {
invalidate_caches(conn, credential);
authenticate_stream(conn, credential, server_api, None).await
}

pub(crate) async fn authenticate_stream(
conn: &mut Connection,
credential: &Credential,
Expand Down
27 changes: 26 additions & 1 deletion src/client/executor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,8 @@ impl Client {
}

/// Selects a server and executes the given operation on it, optionally using a provided
/// session. Retries the operation upon failure if retryability is supported.
/// session. Retries the operation upon failure if retryability is supported or after
/// reauthenticating if reauthentication is required.
async fn execute_operation_with_retry<T: Operation>(
&self,
mut op: T,
Expand Down Expand Up @@ -397,6 +398,30 @@ impl Client {
implicit_session,
},
Err(mut err) => {
// If the error is a reauthentication required error, we reauthenticate and
// retry the operation.
if err.is_reauthentication_required() {
let credential = self.inner.options.credential.as_ref().ok_or(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

both of these errors should be impossible under the current sever spec, but this does future proof us.

ErrorKind::Authentication {
message: "No Credential when reauthentication required error \
occured"
.to_string(),
},
)?;
let server_api = self.inner.options.server_api.as_ref();

credential
.mechanism
.as_ref()
.ok_or(ErrorKind::Authentication {
message: "No AuthMechanism when reauthentication required error \
occured"
.to_string(),
})?
.reauthenticate_stream(&mut conn, credential, server_api)
.await?;
continue;
}
err.wire_version = conn.stream_description()?.max_wire_version;

// Retryable writes are only supported by storage engines with document-level
Expand Down
6 changes: 6 additions & 0 deletions src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ const RETRYABLE_WRITE_CODES: [i32; 12] = [
11600, 11602, 10107, 13435, 13436, 189, 91, 7, 6, 89, 9001, 262,
];
const UNKNOWN_TRANSACTION_COMMIT_RESULT_LABEL_CODES: [i32; 3] = [50, 64, 91];
const REAUTHENTICATION_REQUIRED_CODE: i32 = 391;

/// Retryable write error label. This label will be added to an error when the error is
/// write-retryable.
Expand Down Expand Up @@ -378,6 +379,11 @@ impl Error {
.unwrap_or(false)
}

/// If this error corresponds to a "reauthentication required" error.
pub(crate) fn is_reauthentication_required(&self) -> bool {
self.sdam_code() == Some(REAUTHENTICATION_REQUIRED_CODE)
}

/// If this error corresponds to a "node is recovering" error as per the SDAM spec.
pub(crate) fn is_recovering(&self) -> bool {
self.sdam_code()
Expand Down
46 changes: 31 additions & 15 deletions src/test/spec/oidc.rs
Original file line number Diff line number Diff line change
@@ -1,14 +1,17 @@
use crate::{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided to correct these two tests in this PR as a start on the rest of the prose tests. These tests need to assert the number of times the callback is called.... getting a shared Arc into closures with async blocks is nearly impossible. There does not appear to be a way to do it without the top level function here.

Copy link
Contributor

Choose a reason for hiding this comment

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

The ownership here is gnarly, but it can be made to work: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=437cdfd395260f80a01ddce572d97d52

The trick is that you need to clone the Arc twice: once to give an owned value to the Fn, and a second time within that callback to give an owned value to the Future. That second clone is needed because the outer callback can be called multiple times, and each time it has to produce a future that owns the value, so each time it's called it has to re-clone the Arc.

This can be boiled down a little bit to remove the async aspect, which makes it easier to see what's going on: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=5f8a27ba1ae567f9ba337489d6a49a40

This is also why the distinction between Fn and FnOnce exists; if the Rust compiler knows that the closure can be called at most once, it's fine with transferring ownership from the closure to its return value: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=44aeb2634aa8e87c43e86ea349106f6a

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting. I thought I tried cloning twice and that was what was really shocking to me. I must be misremembering

client::{
auth::{oidc, AuthMechanism, Credential},
options::ClientOptions,
},
test::log_uncaptured,
Client,
};
use std::sync::{Arc, Mutex};

// Machine Callback tests
// Prose test 1.1 Single Principal Implicit Username
#[tokio::test]
async fn single_principal_implicit_username() -> anyhow::Result<()> {
use crate::{
client::{
auth::{oidc, AuthMechanism, Credential},
options::ClientOptions,
},
test::log_uncaptured,
Client,
};
async fn machine_single_principal_implicit_username() -> anyhow::Result<()> {
use bson::Document;
use futures_util::FutureExt;

Expand All @@ -17,10 +20,16 @@ async fn single_principal_implicit_username() -> anyhow::Result<()> {
return Ok(());
}

// we need to assert that the callback is only called once
let call_count = Arc::new(Mutex::new(0));
let cb_call_count = call_count.clone();

let mut opts = ClientOptions::parse("mongodb://localhost/?authMechanism=MONGODB-OIDC").await?;
opts.credential = Credential::builder()
.mechanism(AuthMechanism::MongoDbOidc)
.oidc_callback(oidc::Callback::machine(|_| {
.oidc_callback(oidc::Callback::machine(move |_| {
let call_count = cb_call_count.clone();
*call_count.lock().unwrap() += 1;
async move {
Ok(oidc::IdpServerResponse {
access_token: tokio::fs::read_to_string("/tmp/tokens/test_user1").await?,
Expand All @@ -38,14 +47,14 @@ async fn single_principal_implicit_username() -> anyhow::Result<()> {
.collection::<Document>("test")
.find_one(None, None)
.await?;
assert_eq!(1, *(*call_count).lock().unwrap());
Ok(())
}

// TODO RUST-1497: The following test will be removed because it is not an actual test in the spec,
// but just showing that the human flow is still working for two_step (nothing in caching is
// correctly exercised here)
// Human Callback tests
// Prose test 1.1 Single Principal Implicit Username
#[tokio::test]
async fn human_flow() -> anyhow::Result<()> {
async fn human_single_principal_implicit_username() -> anyhow::Result<()> {
use crate::{
client::{
auth::{oidc, AuthMechanism, Credential},
Expand All @@ -62,10 +71,16 @@ async fn human_flow() -> anyhow::Result<()> {
return Ok(());
}

// we need to assert that the callback is only called once
let call_count = Arc::new(Mutex::new(0));
let cb_call_count = call_count.clone();

let mut opts = ClientOptions::parse("mongodb://localhost/?authMechanism=MONGODB-OIDC").await?;
opts.credential = Credential::builder()
.mechanism(AuthMechanism::MongoDbOidc)
.oidc_callback(oidc::Callback::human(|_| {
.oidc_callback(oidc::Callback::human(move |_| {
let call_count = cb_call_count.clone();
*call_count.lock().unwrap() += 1;
async move {
Ok(oidc::IdpServerResponse {
access_token: tokio::fs::read_to_string("/tmp/tokens/test_user1").await?,
Expand All @@ -83,5 +98,6 @@ async fn human_flow() -> anyhow::Result<()> {
.collection::<Document>("test")
.find_one(None, None)
.await?;
assert_eq!(1, *(*call_count).lock().unwrap());
Ok(())
}