From 756f790d17f0e6dc429a40c3eb67cae709bd1f2d Mon Sep 17 00:00:00 2001
From: Georgy Moshkin <gmoshkin@picodata.io>
Date: Wed, 31 Jan 2024 13:39:33 +0300
Subject: [PATCH] fix: used to panic when checking login attempts via
underprivileged user
---
src/access_control.rs | 18 ++++++++++++++++++
src/lib.rs | 26 ++++++++++++++------------
2 files changed, 32 insertions(+), 12 deletions(-)
diff --git a/src/access_control.rs b/src/access_control.rs
index ba40ce4102..6156a69582 100644
--- a/src/access_control.rs
+++ b/src/access_control.rs
@@ -31,6 +31,7 @@ use std::{
fmt::Display,
};
+use tarantool::index::Index;
use tarantool::{
access_control::{
box_access_check_ddl, box_access_check_space, PrivType,
@@ -117,6 +118,23 @@ pub fn user_by_id(id: UserId) -> tarantool::Result<UserMetadata> {
}
}
+pub fn user_by_name(name: &str) -> tarantool::Result<UserMetadata> {
+ const USER_INDEX_NAME: u32 = 2;
+ // SAFETY: check `box.space._user.index.name.id == 2`
+ let index_user_by_name =
+ unsafe { Index::from_ids_unchecked(SystemSpace::User as _, USER_INDEX_NAME) };
+
+ let Some(tuple) = index_user_by_name.get(&(name,))? else {
+ tarantool::set_error!(
+ tarantool::error::TarantoolErrorCode::NoSuchUser,
+ "no such user '{name}'",
+ );
+ return Err(tarantool::error::TarantoolError::last().into());
+ };
+
+ tuple.decode()
+}
+
/// There are no cases when box_access_check_ddl is called several times
/// in a row so it is ok that we need to switch once to user who initiated the request
/// This wrapper is needed because usually before checking permissions we need to
diff --git a/src/lib.rs b/src/lib.rs
index 16aaa5cbc1..6e1e70eee9 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -20,6 +20,7 @@ use storage::Clusterwide;
use traft::RaftSpaceAccess;
use crate::access_control::user_by_id;
+use crate::access_control::user_by_name;
use crate::cli::args;
use crate::cli::args::Address;
use crate::cli::init_cfg::InitCfg;
@@ -299,26 +300,27 @@ fn set_login_attempts_check(storage: Clusterwide) {
}
// Determines the outcome of an authentication attempt.
- let compute_auth_verdict = move |user: String, status: bool| {
+ let compute_auth_verdict = move |user: String, successful_authentication: bool| {
use std::collections::hash_map::Entry;
let max_login_attempts = || {
- storage
- .properties
- .max_login_attempts()
- .expect("accessing storage should not fail")
+ session::with_su(ADMIN_ID, || {
+ storage
+ .properties
+ .max_login_attempts()
+ .expect("accessing storage should not fail")
+ })
+ .expect("switching user to admin shouldn't fail")
};
- let user_exists = storage
- .users
- .by_name(&user)
- .expect("accessing storage should not fail")
- .is_some();
+ let user_exists = session::with_su(ADMIN_ID, || user_by_name(&user).is_ok())
+ .expect("switching user to admin shouldn't fail");
// Prevent DOS attacks by first checking whether the user exists.
// If it doesn't, we shouldn't even bother tracking its attempts.
// Too many hashmap records will cause a global OOM event.
if !user_exists {
+ debug_assert!(!successful_authentication);
return Verdict::UnknownUser;
}
@@ -333,7 +335,7 @@ fn set_login_attempts_check(storage: Clusterwide) {
Verdict::UserBlocked
}
Entry::Occupied(mut e) => {
- if status {
+ if successful_authentication {
// Forget about previous failures
e.remove();
Verdict::AuthOk
@@ -343,7 +345,7 @@ fn set_login_attempts_check(storage: Clusterwide) {
}
}
Entry::Vacant(e) => {
- if status {
+ if successful_authentication {
Verdict::AuthOk
} else {
// Remember the failure, but don't raise an error yet
--
GitLab