In many of my personal projects, I have a login system very similar to the one I am about to describe. I understand how difficult correctly implementing authorization systems can be. I do this as a hobby and if I can do this securely myself, I would very much like to. (At work, I avoid homebrewed login solutions). I want to know if my approach is secure, and how I can improve it. I am using PHP 5.6 and the CodeIgniter 3 framework.On register, I use password_hash with PASSWORD_BCRYPT.Usernames must be unique, case insensitive (john and jOhN can't bothexist).I only allow an IP to login x amount of time in x minutes (failed orpass because alt accounts are usually discouraged). I have similarrestrictions on registering accounts.I set a max password length of 64 and perform trim on start and end.On login, I use password_verify to compare database result ofusername row with user input.For completeness, here is the most relevant (but not all) parts of auth code.This is my controllerpublic function login(){    // Check if this is ip has logged in too many times    $ip = $_SERVER['REMOTE_ADDR'];    $timestamp = date('Y-m-d H:i:s', time() - $this->login_limit_window * 60);    $route_url = 'user/login';    $check_request_results = $this->main_model->check_request_route($ip, $route_url, $timestamp);    if (count($check_request_results) > $this->login_limit && !is_dev()) {        echo 'Too many login attempts from this IP. Please wait ' . $this->login_limit_window . ' minutes.';        die();    }    // Validation    $this->load->library('form_validation');    $this->form_validation->set_rules('username', 'Username', 'trim|required|max_length[32]');    $this->form_validation->set_rules('password', 'Password', 'trim|required|max_length[64]|callback_login_validation');    // On fail set fail message and redirect to map    if ($this->form_validation->run() == FALSE) {        $this->session->set_flashdata('failed_form', 'login');        $this->session->set_flashdata('validation_errors', validation_errors());        redirect(base_url(), 'refresh');        return false;    }    // Success    redirect(base_url(), 'refresh');}// Validate Login Callbackpublic function login_validation($password){    // Get other parameters    $username = $this->input->post('username');    // Compare to database    $result = $this->user_model->login($username, $password);    // Username not found    if (!$result) {        $this->form_validation->set_message('login_validation', 'Invalid username or password');        return false;    }    // Password does not match    else if (!password_verify($password, $result['password'])) {        $this->form_validation->set_message('login_validation', 'Invalid username or password');        return false;    }    // Success, do login    $sess_array = array(        'id' => $result['id'],        'username' => $result['username']    );    $this->session->set_userdata('logged_in', $sess_array);    return true;}And this is my model// Login database callfunction login($username, $password){   $this->db->select('*');   $this->db->from('user');   $this->db->where('username', $username);   $this->db->limit(1);   $query = $this->db->get();   if ($query->num_rows() == 1) {       $result = $query->result_array();       return isset($result[0]) ? $result[0] : false;   }   else {       return false;   }}
Login implementation security concerns
php;security;authentication;codeigniter
Not a comprehensive review, but I'll list a few things which came to my notice right away:Define the login_limit variable as login_limit_count to keep it consistent with login_limit_window.Instead of defining the above 2 as class attributes, define them as constants in config/constants.php file.Use === in the condition: ($this->form_validation->run() == FALSE)The login method inside your user_model doesn't actually log-in the user. It just fetches the user details based off of $username. No need to provide it with $password and rename it to get_user or similar. Alternatively, put the password_verify inside the model, and return a user on successful login.A select * is very bad. You should always know what values you'd be using from the result.Hardcoding $route_url inside each controller method, and then calling check_request_route can be avoided using $_SERVER['REQUEST_URI'] variable and defining a hook. The hooks need to be enabled first in config/config.php. This way, you can track visits to multiple pages from a single location.