RUST-1662: Correctly handle Client and Connection caching, add Speculative Auth #1040

pmeredit · 2024-03-05T22:45:49Z

No description provided.

…ot be negative

pmeredit · 2024-03-05T22:46:19Z

.evergreen/config.yml


  - name: oidc
    display_name: OIDC
-    patchable: false


This will be removed before merge, again :)

pmeredit · 2024-03-05T22:50:02Z

src/client/auth/oidc.rs

    refresh_token: Option<String>,
    access_token: Option<String>,
-    token_gen_id: i32,
+    token_gen_id: u32,


u32 is the more correct type here, as this should never be negative

pmeredit · 2024-03-05T22:50:10Z

src/client/auth/oidc.rs

 pub struct CallbackContext {
    pub timeout_seconds: Option<Instant>,
-    pub version: i32,
+    pub version: u32,


pmeredit · 2024-03-05T22:51:08Z

src/client/auth/oidc.rs

+    conn: &Connection,
    credential: &Credential,
    response: &IdpServerResponse,
-    token_gen_id: i32,


spec requires caching the IdP info to avoid asking the server for it when it is known (cached).

pmeredit · 2024-03-05T22:52:13Z

src/client/auth/oidc.rs

+        .write()
+        .await;
+    if cache.access_token == Some(access_token) {
+        cache.access_token = None;


I like to use let _ to be very explicit that it's being thrown away

src/client/auth/oidc.rs

pmeredit · 2024-03-05T23:00:17Z

src/client/auth/oidc.rs

+        };
+        // Update the credential and connection caches with the access token and the credential
+        // cache with the refresh token and token_gen_id
+        update_caches(conn, credential, &idp_response, None).await;


The server info should have been set at some time in the past, but also we don't have it here except for whatever was already in the cache, so we just don't update it here. Updating something with itself is just a waste of time.

pmeredit · 2024-03-05T23:05:38Z

src/client/auth/oidc.rs

    pub refresh_token: Option<String>,
 }

+/// Constructs the first client message in the OIDC handshake for speculative authentication


Is it ok for this to return an empty speculative command when there is no access token, or should we actually make this return an Option?

I think it's fine to return an empty command as long as the calling code doesn't have to special case that.

…small

… tokio

pmeredit · 2024-03-06T03:09:25Z

.evergreen/config.yml

+    patchable: true
    run_on:
-      - rhel87-small
+      - ubuntu2204-small


This was just so the oidc tests would start immediately

…llback

src/client/auth/oidc.rs

…are in synch

pmeredit · 2024-03-06T18:03:14Z

src/cmap/conn.rs

    compression::Compressor,
    error::{load_balanced_mode_mismatch, Error, ErrorKind, Result},
    event::cmap::{
-        CmapEventEmitter, ConnectionCheckedInEvent, ConnectionCheckedOutEvent,


Looks like somehow the rustfmt I run on nvim buffer save is not the correct version of rustfmt.

abr-egn · 2024-03-06T19:40:13Z

src/client/auth/oidc.rs

 #[derive(Clone)]
 pub struct State {
    callback: Callback,
    cache: Arc<RwLock<Cache>>,


Does this need to be held across await points? If not, this could become a std::sync::RwLock and avoid the async that has to propagate in various places here.

Let me check

Looks like it will work. I didn't realize that it was safe to use std::sync::RwLock as long as it doesn't cross await, but that makes sense. I assume the only difference of the tokio version is it handles unlocking when the await is rescheduled.

Yup, exactly.

abr-egn

LGTM!

abr-egn · 2024-03-06T20:09:35Z

src/client/auth/oidc.rs

 #[derive(Clone)]
 pub struct State {
    callback: Callback,
    cache: Arc<RwLock<Cache>>,


Yup, exactly.

abr-egn · 2024-03-06T20:10:38Z

src/client/auth/oidc.rs

    pub refresh_token: Option<String>,
 }

+/// Constructs the first client message in the OIDC handshake for speculative authentication


I think it's fine to return an empty command as long as the calling code doesn't have to special case that.

pmeredit · 2024-03-06T20:17:29Z

@abr-egn unfortunately, the calling code would have to special case it if returned the Option. I think this approach is fine because the worst that happens is we send nonsense (e.g., an empty or missing jwt) in the speculative auth which just gets returned as nothing and then we do normal auth. Seems like there's no downside to that, and then we avoid the special casing.

isabelatkinson

just two very small suggestions, no need for a re-review!

isabelatkinson · 2024-03-06T21:58:11Z

src/cmap/establish/handshake.rs


 /// Updates the handshake command document with the speculative authenitication info.
-fn set_speculative_auth_info(
+async fn set_speculative_auth_info(


Is async code going to be added to this method? If not recommend reverting this change

Nope! I just missed this one, thanks!

isabelatkinson · 2024-03-06T22:26:42Z

src/client/auth/oidc.rs

+
+    if credential.oidc_callback.is_none() {
+        auth_command_doc.insert("jwt", "");
+    } else if let Some(access_token) = credential


Suggested change

} else if let Some(access_token) = credential

} else if let Some(access_token) = credential.get_access_token()

(looks like this chain of method calls is the same as the contents of get_access_token)

Yep! Great call!

pmeredit added 9 commits March 5, 2024 12:56

RUST-1662: Initial caching checkpoint

db62e00

RUST-1662: Finish non-speculative initial authentication

5738c04

RUST-1662: Refactor out two_step_auth

e0ba578

RUST-1662: Use constants, use u32 instead of i32 for things that cann…

3d73dd0

…ot be negative

RUST-1662: Add speculative command building

014ae5d

RUST-1662: Update validation and and comments

43f934a

RUST-1662: Make oidc test patchable

cf0ad9f

RUST-1662: Add human test

6cf4d20

RUST-1662: Update error

db511b2

pmeredit commented Mar 5, 2024

View reviewed changes

Update src/client/auth/oidc.rs

cff6abc

pmeredit commented Mar 5, 2024

View reviewed changes

pmeredit added 6 commits March 5, 2024 18:19

RUST-1662: Fix problems with spec auth

80f8b06

RUST-1662: Minor cleanup

b3cf7f5

RUST-1662: Why is validate failing in the tests?

fe22a70

RUST-1662: use multithreaded?

a58357e

RUST-1662: Ok, looks like we need to propagate async. The change was …

376b2f3

…small

RUST-1662: See if the validate failure was just caused by a misuse of…

b958989

… tokio

pmeredit commented Mar 6, 2024

View reviewed changes

pmeredit added 2 commits March 5, 2024 22:50

RUST-1662: Add callback so validate doesn't fail

faee44e

RUST-1662: Do better, don't add empty callback, also don't require ca…

df0fd63

…llback

pmeredit requested review from abr-egn and isabelatkinson March 6, 2024 04:31

pmeredit commented Mar 6, 2024

View reviewed changes

src/client/auth/oidc.rs Outdated Show resolved Hide resolved

pmeredit and others added 5 commits March 5, 2024 23:35

Update src/client/auth/oidc.rs

900c9e6

RUST-1662: Refactor and ensure that idpserver info and refresh token …

0c909bd

…are in synch

RUST-1662: Change to use token_gen_id like the python driver

a8c3588

RUST-1662: Use consistent naming between the two cache functions

f5c20e2

RUST-1662: rustfmt?

6a527a0

pmeredit commented Mar 6, 2024

View reviewed changes

abr-egn reviewed Mar 6, 2024

View reviewed changes

RUST-1662: Use std::sync::RwLock for Client Cache

e80dc57

abr-egn approved these changes Mar 6, 2024

View reviewed changes

RUST-1662: Connection Cache can also be sync since it's not public

a7099b4

RUST-1662: Looks like the rustfmt problem is with stable vs nightly

ce319ad

isabelatkinson approved these changes Mar 6, 2024

View reviewed changes

RUST-1662: Fix unnecessary async, reuse method

b3e8be5

pmeredit merged commit a2faea4 into mongodb:main Mar 6, 2024

	} else if let Some(access_token) = credential
	} else if let Some(access_token) = credential.get_access_token()

RUST-1662: Correctly handle Client and Connection caching, add Speculative Auth #1040

RUST-1662: Correctly handle Client and Connection caching, add Speculative Auth #1040

Uh oh!

Conversation

pmeredit commented Mar 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmeredit Mar 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abr-egn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmeredit commented Mar 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

isabelatkinson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pmeredit Mar 6, 2024 •

edited

Loading

pmeredit commented Mar 6, 2024 •

edited

Loading