Introduce UserAuthStrategy to allow third party authentication implementation #1026

jameswritescode · 2024-02-13T21:08:15Z

What are you trying to accomplish?

This PR seeks to open up authentication so that anyone using libsql-server as a library can provide their own authentication strategy.

What approach did you choose and why?

We removed the Auth struct in favor of structs that implement UserAuthStrategy that can be configured via UserApiConfig. The existing strategies of HTTP Basic and JWT have been moved into their own strategy definitions. Anywhere Auth was previously being call for authentication has been switched out with UserAuthStrategy#authenticate.

What should reviewers focus on?

A handful of the changes in the auth mod with exception of the auth strategies are existing code from the original auth.rs but put into their own files.
We would like to follow up in the future with changes to AuthError to provide more generic enum variants with data to give auth strategies more control over their errors.
How do we feel about the changes in hrana::ws? How might we approach this differently otherwise?

penberg · 2024-02-16T11:06:43Z

Looks good to me, but @MarinPostma or @LucioFranco ought to review this before merging.

MarinPostma · 2024-02-16T15:42:54Z

I need a bit of time to look at this, but will do asap

LucioFranco

Overall this seems like a great improvement, I left a few comments/questions regarding some style things and api designs. Let me know if something isn't clear! thanks!

libsql-server/src/auth/mod.rs

libsql-server/src/auth/user_auth_strategies/disabled.rs

LucioFranco · 2024-02-16T15:34:52Z

libsql-server/src/auth/user_auth_strategies/mod.rs

+}
+
+pub trait UserAuthStrategy: Sync + Send {
+    fn authenticate(&self, context: UserAuthContext) -> Result<Authenticated, AuthError>;


I wonder if we can just accept a borrow here, I believe no auth really needs full ownership of this struct? Its likely we don't need this now but its good to encode that contract in a trait if we are going to abstract it.

If we keep the struct around, can change!

LucioFranco · 2024-02-16T15:35:25Z

libsql-server/src/config.rs

@@ -60,8 +60,7 @@ pub struct UserApiConfig<A = AddrIncoming> {
    pub http_acceptor: Option<A>,
    pub enable_http_console: bool,
    pub self_url: Option<String>,
-    pub http_auth: Option<String>,
-    pub auth_jwt_key: Option<String>,
+    pub auth_strategy: Arc<Box<dyn UserAuthStrategy>>,


Whats the reason for a double pointer indirection here? Could we just do Arc<dyn UserAuthStrategy> and avoid the box?

This was my misunderstanding, I'd started by passing a Box and then brought in the Arc, didn't realize that it eliminated the need for Box. Thanks for pointing it out.

libsql-server/src/auth/errors.rs

libsql-server/src/hrana/ws/mod.rs

libsql-server/src/hrana/ws/session.rs

LucioFranco · 2024-02-16T16:58:08Z

libsql-server/src/hrana/ws/session.rs

        .await??;
+
+    // Convert jwt token into a HeaderValue to be compatible with UserAuthStrategy
+    let user_credential = jwt


unfortunate but I think this is fine for now, WS is not our primary protocol anyways for hrana so this I think is acceptable for now.

I think the only thing this really poses is should we accept a String rather than a HeaderValue in the auth trait?

I like the idea of user_credential being an Option<String> though a couple of things off the top of my head I think we'd need to account for:

In HTTP context, do we pass the entire Authorization header value?

In this WS context, do we still prepend an Authorization scheme or does the strategy need to do some extra homework to determine if it's being given a header value or something else?

I think it would make sense to strip the prefix at the http/ws level and then only pass through the "token" to the Auth layer? I feel like that would make the most sense. What do you think? That way we can just set this as a string/bytes and not leak HeaderValue through to the auth layer.

@shopifyski Said he'd take this on as a follow up PR!

…entation Co-authored-by: Jeremy Rowe <[email protected]>

LucioFranco

Overall looks good left a question and some feedback, we can merge this and then do that as follow up or yall could implement those changes and we can merge up to you.

LucioFranco · 2024-02-20T22:17:44Z

libsql-server/src/auth/mod.rs

+pub use permission::Permission;
+pub use user_auth_strategies::{Disabled, HttpBasic, Jwt, UserAuthContext, UserAuthStrategy};
+
+pub type Auth = Arc<dyn UserAuthStrategy>;


I think we can do this in a follow up and the code change should be pretty small, but I would like this to be a struct since type aliases tend to leak the internal implementation detail. So it could be a struct like:

#[derive(Clone)] pub struct Auth { inner: Arc<dyn UserAuthStrategy + Send + Sync>, } impl Auth { // implement functions that we need to be public + constructor via `impl UserAuthStrategy` }

I'd like to take care of this in this pull request. I'll take a stab at it right now :D

This is taken care of here ad18ef0

LucioFranco · 2024-02-20T22:20:23Z

libsql-server/src/hrana/ws/session.rs

        .await??;
+
+    // Convert jwt token into a HeaderValue to be compatible with UserAuthStrategy
+    let user_credential = jwt


I think it would make sense to strip the prefix at the http/ws level and then only pass through the "token" to the Auth layer? I feel like that would make the most sense. What do you think? That way we can just set this as a string/bytes and not leak HeaderValue through to the auth layer.

jeremywrowe · 2024-02-21T18:21:31Z

libsql-server/src/auth/mod.rs

+pub use permission::Permission;
+pub use user_auth_strategies::{Disabled, HttpBasic, Jwt, UserAuthContext, UserAuthStrategy};
+
+#[derive(Clone)]


TODO: Add a comment on why we chose a struct over a type alias.

MarinPostma · 2024-02-22T10:42:41Z

folks, since @LucioFranco only has nits, and I need to do auth work, I'm merging this now, I'll let you address the rest in followups :)

let's address the rest in followups

jameswritescode force-pushed the auth-strategies branch 2 times, most recently from 41fd70c to b3b3c75 Compare February 14, 2024 18:46

penberg requested review from MarinPostma and LucioFranco February 16, 2024 11:05

LucioFranco previously requested changes Feb 16, 2024

View reviewed changes

jameswritescode force-pushed the auth-strategies branch 2 times, most recently from 9ccea41 to 5b0c509 Compare February 16, 2024 19:40

Introduce UserAuthStrategy to allow third party authentication implem…

388c3c9

…entation Co-authored-by: Jeremy Rowe <[email protected]>

jameswritescode force-pushed the auth-strategies branch from 5b0c509 to 388c3c9 Compare February 16, 2024 21:31

LucioFranco reviewed Feb 20, 2024

View reviewed changes

PR feedback

ad18ef0

jeremywrowe reviewed Feb 21, 2024

View reviewed changes

MarinPostma mentioned this pull request Feb 22, 2024

auth strategy2 #1072

Merged

MarinPostma closed this in #1072 Feb 22, 2024

jeremywrowe deleted the auth-strategies branch February 22, 2024 15:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce UserAuthStrategy to allow third party authentication implementation #1026

Introduce UserAuthStrategy to allow third party authentication implementation #1026

jameswritescode commented Feb 13, 2024

penberg commented Feb 16, 2024

MarinPostma commented Feb 16, 2024

LucioFranco left a comment

LucioFranco Feb 16, 2024

jameswritescode Feb 16, 2024

LucioFranco Feb 16, 2024

jameswritescode Feb 16, 2024

LucioFranco Feb 16, 2024

jameswritescode Feb 16, 2024 •

edited

Loading

LucioFranco Feb 20, 2024

jeremywrowe Feb 21, 2024

LucioFranco left a comment

LucioFranco Feb 20, 2024

jeremywrowe Feb 21, 2024

jeremywrowe Feb 21, 2024

LucioFranco Feb 20, 2024

jeremywrowe Feb 21, 2024

MarinPostma commented Feb 22, 2024

Introduce UserAuthStrategy to allow third party authentication implementation #1026

Introduce UserAuthStrategy to allow third party authentication implementation #1026

Conversation

jameswritescode commented Feb 13, 2024

What are you trying to accomplish?

What approach did you choose and why?

What should reviewers focus on?

penberg commented Feb 16, 2024

MarinPostma commented Feb 16, 2024

LucioFranco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jameswritescode Feb 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucioFranco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarinPostma commented Feb 22, 2024

jameswritescode Feb 16, 2024 •

edited

Loading