implement login via LDAP
#826
Manually merged
trinity-1686a
merged 6 commits from ldap into main 4 years ago
@ -20,6 +20,7 @@ pub struct Config {
|
||||
pub logo: LogoConfig,
|
||||
pub default_theme: String,
|
||||
pub media_directory: String,
|
||||
pub ldap: Option<LdapConfig>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone)]
|
||||
@ -240,6 +241,40 @@ impl SearchTokenizerConfig {
|
||||
}
|
||||
}
|
||||
|
||||
pub struct LdapConfig {
|
||||
pub addr: String,
|
||||
pub base_dn: String,
|
||||
pub tls: bool,
|
||||
pub user_name_attr: String,
|
||||
pub mail_attr: String,
|
||||
}
|
||||
|
||||
fn get_ldap_config() -> Option<LdapConfig> {
|
||||
let addr = var("LDAP_ADDR").ok();
|
||||
let base_dn = var("LDAP_BASE_DN").ok();
|
||||
if addr.is_some() && base_dn.is_some() {
|
||||
let tls = var("LDAP_TLS").unwrap_or_else(|_| "false".to_owned());
|
||||
let tls = match tls.as_ref() {
|
||||
"1" | "true" | "TRUE" => true,
|
||||
"0" | "false" | "FALSE" => false,
|
||||
_ => panic!("Invalid LDAP configuration : tls"),
|
||||
};
|
||||
let user_name_attr = var("LDAP_USER_NAME_ATTR").unwrap_or_else(|_| "cn".to_owned());
|
||||
let mail_attr = var("LDAP_USER_MAIL_ATTR").unwrap_or_else(|_| "mail".to_owned());
|
||||
Some(LdapConfig {
|
||||
addr: addr.unwrap(),
|
||||
base_dn: base_dn.unwrap(),
|
||||
tls,
|
||||
user_name_attr,
|
||||
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")
|
||||
|
trinity-1686a marked this conversation as resolved
igalic
commented 4 years ago
Review
we panic, because both of those must be set??! we panic, because both of those must be set??!
trinity-1686a
commented 4 years ago
Review
Hum there is a mistake making this code unreachable. If both are set, it will take the first 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
|
||||
} else {
|
||||
None
|
||||
}
|
||||
}
|
||||
|
||||
lazy_static! {
|
||||
pub static ref CONFIG: Config = Config {
|
||||
base_url: var("BASE_URL").unwrap_or_else(|_| format!(
|
||||
@ -267,5 +302,6 @@ lazy_static! {
|
||||
default_theme: var("DEFAULT_THEME").unwrap_or_else(|_| "default-light".to_owned()),
|
||||
media_directory: var("MEDIA_UPLOAD_DIRECTORY")
|
||||
.unwrap_or_else(|_| "static/media".to_owned()),
|
||||
ldap: get_ldap_config(),
|
||||
};
|
||||
}
|
||||
|
||||
@ -1,8 +1,8 @@
|
||||
use crate::{
|
||||
ap_url, blocklisted_emails::BlocklistedEmail, blogs::Blog, db_conn::DbConn, follows::Follow,
|
||||
instance::*, medias::Media, notifications::Notification, post_authors::PostAuthor, posts::Post,
|
||||
safe_string::SafeString, schema::users, search::Searcher, timeline::Timeline, Connection,
|
||||
Error, PlumeRocket, Result, ITEMS_PER_PAGE,
|
||||
ap_url, blocklisted_emails::BlocklistedEmail, blogs::Blog, config::CONFIG, db_conn::DbConn,
|
||||
follows::Follow, instance::*, medias::Media, notifications::Notification,
|
||||
post_authors::PostAuthor, posts::Post, safe_string::SafeString, schema::users,
|
||||
search::Searcher, timeline::Timeline, Connection, Error, PlumeRocket, Result, ITEMS_PER_PAGE,
|
||||
};
|
||||
use activitypub::{
|
||||
activity::Delete,
|
||||
@ -14,6 +14,7 @@ use activitypub::{
|
||||
use bcrypt;
|
||||
use chrono::{NaiveDateTime, Utc};
|
||||
use diesel::{self, BelongingToDsl, ExpressionMethods, OptionalExtension, QueryDsl, RunQueryDsl};
|
||||
use ldap3::{LdapConn, Scope, SearchEntry};
|
||||
use openssl::{
|
||||
hash::MessageDigest,
|
||||
pkey::{PKey, Private},
|
||||
@ -292,11 +293,116 @@ impl User {
|
||||
bcrypt::hash(pass, 10).map_err(Error::from)
|
||||
}
|
||||
|
||||
pub fn auth(&self, pass: &str) -> bool {
|
||||
self.hashed_password
|
||||
.clone()
|
||||
.map(|hashed| bcrypt::verify(pass, hashed.as_ref()).unwrap_or(false))
|
||||
.unwrap_or(false)
|
||||
fn ldap_register(conn: &Connection, name: &str, password: &str) -> Result<User> {
|
||||
if CONFIG.ldap.is_none() {
|
||||
return Err(Error::NotFound);
|
||||
}
|
||||
let ldap = CONFIG.ldap.as_ref().unwrap();
|
||||
|
||||
let mut ldap_conn = LdapConn::new(&ldap.addr).map_err(|_| Error::NotFound)?;
|
||||
let ldap_name = format!("{}={},{}", ldap.user_name_attr, name, ldap.base_dn);
|
||||
let bind = ldap_conn
|
||||
.simple_bind(&ldap_name, password)
|
||||
.map_err(|_| Error::NotFound)?;
|
||||
|
||||
if bind.success().is_err() {
|
||||
return Err(Error::NotFound);
|
||||
}
|
||||
|
||||
let search = ldap_conn
|
||||
.search(
|
||||
&ldap_name,
|
||||
Scope::Base,
|
||||
"(|(objectClass=person)(objectClass=user))",
|
||||
vec![&ldap.mail_attr],
|
||||
)
|
||||
.map_err(|_| Error::NotFound)?
|
||||
.success()
|
||||
.map_err(|_| Error::NotFound)?;
|
||||
for entry in search.0 {
|
||||
let entry = SearchEntry::construct(entry);
|
||||
let email = entry.attrs.get("mail").and_then(|vec| vec.first());
|
||||
if email.is_some() {
|
||||
let _ = ldap_conn.unbind();
|
||||
return NewUser::new_local(
|
||||
conn,
|
||||
name.to_owned(),
|
||||
name.to_owned(),
|
||||
Role::Normal,
|
||||
"",
|
||||
email.unwrap().to_owned(),
|
||||
None,
|
||||
);
|
||||
}
|
||||
|
trinity-1686a marked this conversation as resolved
igalic
commented 4 years ago
Review
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. 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.
|
||||
}
|
||||
let _ = ldap_conn.unbind();
|
||||
Err(Error::NotFound)
|
||||
}
|
||||
|
||||
fn ldap_login(&self, password: &str) -> bool {
|
||||
if let Some(ldap) = CONFIG.ldap.as_ref() {
|
||||
let mut conn = if let Ok(conn) = LdapConn::new(&ldap.addr) {
|
||||
conn
|
||||
} else {
|
||||
return false;
|
||||
};
|
||||
let name = format!(
|
||||
"{}={},{}",
|
||||
ldap.user_name_attr, &self.username, ldap.base_dn
|
||||
);
|
||||
if let Ok(bind) = conn.simple_bind(&name, password) {
|
||||
bind.success().is_ok()
|
||||
} else {
|
||||
false
|
||||
}
|
||||
} else {
|
||||
false
|
||||
}
|
||||
}
|
||||
|
trinity-1686a marked this conversation as resolved
igalic
commented 4 years ago
Review
can you please document where in this code we now fake the login check to prevent timing attacks? 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.
|
||||
|
||||
pub fn login(conn: &Connection, ident: &str, password: &str) -> Result<User> {
|
||||
let local_id = Instance::get_local()?.id;
|
||||
|
igalic marked this conversation as resolved
igalic
commented 4 years ago
Review
these… these…
|
||||
let user = match User::find_by_email(conn, ident) {
|
||||
Ok(user) => Ok(user),
|
||||
_ => User::find_by_name(conn, ident, local_id),
|
||||
}
|
||||
.and_then(|u| {
|
||||
if u.instance_id == local_id {
|
||||
Ok(u)
|
||||
} else {
|
||||
Err(Error::NotFound)
|
||||
}
|
||||
});
|
||||
|
||||
match user {
|
||||
Ok(user) if user.hashed_password.is_some() => {
|
||||
if bcrypt::verify(password, user.hashed_password.as_ref().unwrap()).unwrap_or(false)
|
||||
{
|
||||
Ok(user)
|
||||
} else {
|
||||
Err(Error::NotFound)
|
||||
}
|
||||
}
|
||||
Ok(user) => {
|
||||
if user.ldap_login(password) {
|
||||
Ok(user)
|
||||
} else {
|
||||
Err(Error::NotFound)
|
||||
}
|
||||
}
|
||||
e => {
|
||||
if let Ok(user) = User::ldap_register(conn, ident, password) {
|
||||
return Ok(user);
|
||||
}
|
||||
// if no user was found, and we were unable to auto-register from ldap
|
||||
// fake-verify a password, and return an error.
|
||||
let other = User::get(&*conn, 1)
|
||||
.expect("No user is registered")
|
||||
.hashed_password;
|
||||
other.map(|pass| bcrypt::verify(password, &pass));
|
||||
e
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
pub fn reset_password(&self, conn: &Connection, pass: &str) -> Result<()> {
|
||||
@ -983,7 +1089,7 @@ impl NewUser {
|
||||
role: Role,
|
||||
summary: &str,
|
||||
email: String,
|
||||
password: String,
|
||||
password: Option<String>,
|
||||
) -> Result<User> {
|
||||
let (pub_key, priv_key) = gen_keypair();
|
||||
let instance = Instance::get_local()?;
|
||||
@ -1001,7 +1107,7 @@ impl NewUser {
|
||||
summary: summary.to_owned(),
|
||||
summary_html: SafeString::new(&utils::md_to_html(&summary, None, false, None).0),
|
||||
email: Some(email),
|
||||
hashed_password: Some(password),
|
||||
hashed_password: password,
|
||||
instance_id: instance.id,
|
||||
public_key: String::from_utf8(pub_key).or(Err(Error::Signature))?,
|
||||
private_key: Some(String::from_utf8(priv_key).or(Err(Error::Signature))?),
|
||||
|
||||
Loading…
Reference in New Issue
we should upgrade our own dependencies of these packages to 1.x