implement login via LDAP #826

trinity-1686a · 2020-10-03T11:23:49Z

trinity-1686a commented

2020-10-03 11:23:49 +00:00

fix #312

👍 1

igalic commented

2020-10-03 16:49:58 +00:00

-            // Making fake password verification to avoid different
-            // response times that would make it possible to know
-            // if a username is registered or not.
-            User::get(conn, 1)?.auth(&query.password);

why would you get rid of this?

``` - // Making fake password verification to avoid different - // response times that would make it possible to know - // if a username is registered or not. - User::get(conn, 1)?.auth(&query.password); ``` why would you get rid of this?

trinity-1686a commented

2020-10-03 20:40:20 +00:00

It has been added back in the model, inside login()

trinity-1686a changed title from ~~WIP: implement login via LDAP~~ to implement login via LDAP

2020-10-04 10:19:20 +00:00

trinity-1686a commented

2020-10-04 10:21:45 +00:00

I've tested the implementation against Bottin, however I'm not sure how to add tests that can be run automatically.

Once this is merged, I'll add the new environment variables to the docs.

I've tested the implementation against [Bottin](https://git.deuxfleurs.fr/Deuxfleurs/bottin), however I'm not sure how to add tests that can be run automatically. Once this is merged, I'll add the new environment variables to the docs.

🎉 1

igalic requested changes 2020-10-07 18:33:54 +00:00

igalic left a comment

i've looked at this now and have left a few comments.
I like the general look and feel!

i've looked at this now and have left a few comments. I like the general look and feel!

Cargo.lock

					
				@ -172,0 +176,4 @@

				dependencies = [

				 "proc-macro2 1.0.18 (registry+https://github.com/rust-lang/crates.io-index)",

				 "quote 1.0.7 (registry+https://github.com/rust-lang/crates.io-index)",

				 "syn 1.0.34 (registry+https://github.com/rust-lang/crates.io-index)",

igalic commented

2020-10-07 18:06:42 +00:00

we should upgrade our own dependencies of these packages to 1.x

igalic marked this conversation as resolved

plume-models/src/config.rs Outdated

					
				@ -243,0 +268,4 @@

				            user_name_attr,

				            mail_attr,

				        })

				    } else if addr.is_some() && base_dn.is_some() {

igalic commented

2020-10-07 18:17:22 +00:00

if addr and base_dn is_some()…

if addr and base_dn `is_some()`…

trinity-1686a marked this conversation as resolved

plume-models/src/config.rs

					
				@ -243,0 +269,4 @@

				            mail_attr,

				        })

				    } else if addr.is_some() && base_dn.is_some() {

				        panic!("Invalid LDAP configuration : both LDAP_ADDR and LDAP_BASE_DN must be set")

igalic commented

2020-10-07 18:17:41 +00:00

we panic, because both of those must be set??!

trinity-1686a commented

2020-10-07 21:21:16 +00:00

Hum there is a mistake making this code unreachable. If both are set, it will take the first if, if only one is set, it should take the else if (which is not the case, I'll change that), I consider that invalid as both an addr and a base_dn are required, and there is no good default value (contrary to other vars), and finaly the else is to disable ldap when none is configured

Hum there is a mistake making this code unreachable. If both are set, it will take the first `if`, if only one is set, it should take the `else if` (which is not the case, I'll change that), I consider that invalid as both an addr and a base_dn are required, and there is no good default value (contrary to other vars), and finaly the else is to disable ldap when none is configured

trinity-1686a marked this conversation as resolved

plume-models/src/users.rs Outdated

					
				@ -300,0 +300,4 @@

				            let bind = ldap_conn

				                .simple_bind(&ldap_name, password)

				                .map_err(|_| Error::NotFound)?;

				            if bind.success().is_ok() {

igalic commented

2020-10-07 18:22:38 +00:00

if we use ldap as user-database, we could do the ldap bind once, and then pass around a connection…

…well… an Arc<Mutex<LdapConn>>

if we use ldap as user-database, we could do the ldap bind once, and then pass around a connection… …well… an `Arc<Mutex<LdapConn>>`

trinity-1686a commented

2020-10-07 21:23:02 +00:00

We could, that's more or less what I was doing on a draft a few months ago, but I lost it, and I'm not sure how much it would be helpful. Sure we use it as a user database, but we only consult it on login/account creation, so not exactly a bottleneck

plume-models/src/users.rs

					
				@ -300,0 +333,4 @@

				                Err(Error::NotFound)

				            }

				        } else {

				            Err(Error::NotFound)

igalic commented

2020-10-07 18:25:18 +00:00

would it be possible to do these as if not tests, so we don't nest four levels deep until we get to the actual code.

trinity-1686a marked this conversation as resolved

plume-models/src/users.rs

					
				@ -300,0 +358,4 @@

				        }

				    }

				    pub fn login(conn: &Connection, ident: &str, password: &str) -> Result<User> {

igalic commented

2020-10-07 18:32:07 +00:00

can you please document where in this code we now fake the login check to prevent timing attacks?
i can't seem to see it in this code.

can you please document where in this code we now fake the login check to prevent timing attacks? i can't seem to see it in this code.

trinity-1686a marked this conversation as resolved

plume-models/src/users.rs

					
				@ -300,0 +361,4 @@

				    pub fn login(conn: &Connection, ident: &str, password: &str) -> Result<User> {

				        let local_id = Instance::get_local()?.id;

				        let user = User::find_by_email(conn, ident)

				            .or_else(|_| User::find_by_name(conn, ident, local_id))

igalic commented

2020-10-07 18:27:31 +00:00

these…

igalic marked this conversation as resolved

plume-models/src/users.rs Outdated

					
				@ -300,0 +362,4 @@

				        let local_id = Instance::get_local()?.id;

				        let user = User::find_by_email(conn, ident)

				            .or_else(|_| User::find_by_name(conn, ident, local_id))

				            .and_then(|u| {