Simple administrator authentication page in PHP

Question

I made a simple administrator page lock in PHP and I was wondering how secure it was. I want to use this script to secure administrator pages for my portfolio site but I want it to be fairly secure.

<?php

/*
 * Array of authenticated users
 */

$users = [
  // Username => Password
  'admin' => 'password'
];

/*
 * Expiration time in seconds
 */

$expire = 30 * 60;

/*
 * We take care of the rest
 */

// Start the session
session_start();

// Last session activity time is greater than the set expiration time
if (isset($_SESSION['LAST_ACTIVITY']) && (time() - $_SESSION['LAST_ACTIVITY'] > $expire)) {
  // Free all session variables
  session_unset();
  // Destroy all session variables
  session_destroy();
}

// Set last session activity time
$_SESSION['LAST_ACTIVITY'] = time();

// On form submission
if (isset($_POST['username'], $_POST['password'])) {
  // Reference username and password
  $username = $_POST['username'];
  $password = $_POST['password'];
  // Provided username exists
  if (array_key_exists($username, $users)) {
    // Provided password is verified
    if ($password == $users[$username]) {
      // Set the session user identifier
      $_SESSION['USER_ID'] = $username;
    }
  }
}

// User identifier is not present or is valid
if (!isset($_SESSION['USER_ID']) || !array_key_exists($_SESSION['USER_ID'], $users)) {
  // Render the form for authentication attempts
  echo '<form method="post"><div><input type="text" name="username" placeholder="Username"></div><div><input type="password" name="password" placeholder="Password"></div><div><input type="submit"></div></form>';
  // Don't allow any action until authentication is finished
  exit();
}

?>

I plan on adding things like halting the script for a few seconds depending on how many attempts were made from the current session.

Any suggestions or insights on if this is secure or how to make it more secure would be very helpful.

Answer 1

You should not store your password in plaintext, as this means that read access (eg via LFI) automatically leads to more (probably some sort of write access via the admin backend). Use bcrypt for this (this also solves the next point automatically).

Ideally, you should also use some sort of timing safe string comparison.

Ideally, I would move the user credentials into an extra config file, which can be placed outside the web root (in case the connection breaks up while a user edits the file, which may lead to a backup file like login.php~ or similar being created, which may be severed by a webserver).

I plan on adding things like halting the script for a few seconds depending on how many attempts were made from the current session.

I guess that is one solution, but I would just deny attempts completely for x seconds if y attempts where made from that IP in the last z seconds.

Misc

• You are overdoing it a bit with comments, which leads people to ignore your comments. Comments should not rephrase what the code already told us, but add additional information in case the code is unclear (Username => Password and possibly Expiration time in seconds/Array of authenticated users are the only comments I would actually keep).