Authentication tests, and plain 'password' authentication with a SCRAM verifier

From fd200d76b40736af8ee98b09ddaf62ddc5ed9d72 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Tue, 14 Mar 2017 14:26:50 +0200
Subject: [PATCH 1/2] Allow plaintext 'password' authentication when user has a
 SCRAM verifier.

---
 src/backend/libpq/auth-scram.c | 46 +++++++++++++++++++++++++++++++
 src/backend/libpq/crypt.c      | 62 +++++++++++++++++++++++++++---------------
 src/include/libpq/scram.h      |  2 ++
 3 files changed, 88 insertions(+), 22 deletions(-)

diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c
index 9f78e57aae..0833759afb 100644
--- a/src/backend/libpq/auth-scram.c
+++ b/src/backend/libpq/auth-scram.c
@@ -364,6 +364,52 @@ scram_build_verifier(const char *username, const char *password,
 	return psprintf("scram-sha-256:%s:%d:%s:%s", encoded_salt, iterations, storedkey_hex, serverkey_hex);
 }
 
+/*
+ * Verify a plaintext password against a SCRAM verifier. This is used when
+ * performing plaintext password authentication for a user that has a SCRAM
+ * verifier stored in pg_authid.
+ */
+bool
+scram_verify_plain_password(const char *username, const char *password,
+							const char *verifier)
+{
+	char	   *encoded_salt;
+	char	   *salt;
+	int			saltlen;
+	int			iterations;
+	uint8		stored_key[SCRAM_KEY_LEN];
+	uint8		server_key[SCRAM_KEY_LEN];
+	uint8		computed_key[SCRAM_KEY_LEN];
+
+	if (!parse_scram_verifier(verifier, &encoded_salt, &iterations,
+							  stored_key, server_key))
+	{
+		/*
+		 * The password looked like a SCRAM verifier, but could not be
+		 * parsed.
+		 */
+		elog(LOG, "invalid SCRAM verifier for user \"%s\"", username);
+		return false;
+	}
+
+	salt = palloc(pg_b64_dec_len(strlen(encoded_salt)));
+	saltlen = pg_b64_decode(encoded_salt, strlen(encoded_salt), salt);
+	if (saltlen == -1)
+	{
+		elog(LOG, "invalid SCRAM verifier for user \"%s\"", username);
+		return false;
+	}
+
+	/* Compute Server key based on the user-supplied plaintext password */
+	scram_ClientOrServerKey(password, salt, saltlen, iterations,
+							SCRAM_SERVER_KEY_NAME, computed_key);
+
+	/*
+	 * Compare the verifier's Server Key with the one computed from the
+	 * user-supplied password.
+	 */
+	return memcmp(computed_key, server_key, SCRAM_KEY_LEN) == 0;
+}
 
 /*
  * Check if given verifier can be used for SCRAM authentication.
diff --git a/src/backend/libpq/crypt.c b/src/backend/libpq/crypt.c
index 9f0ae15b00..3ec0fd7b92 100644
--- a/src/backend/libpq/crypt.c
+++ b/src/backend/libpq/crypt.c
@@ -283,7 +283,6 @@ plain_crypt_verify(const char *role, const char *shadow_pass,
 				   const char *client_pass,
 				   char **logdetail)
 {
-	int			retval;
 	char		crypt_client_pass[MD5_PASSWD_LEN + 1];
 
 	/*
@@ -293,6 +292,22 @@ plain_crypt_verify(const char *role, const char *shadow_pass,
 	 */
 	switch (get_password_type(shadow_pass))
 	{
+		case PASSWORD_TYPE_SCRAM:
+			if (scram_verify_plain_password(role,
+											client_pass,
+											shadow_pass))
+			{
+				return STATUS_OK;
+			}
+			else
+			{
+				if (!*logdetail)
+					*logdetail = psprintf(_("Password does not match for user \"%s\"."),
+										  role);
+				return STATUS_ERROR;
+			}
+			break;
+
 		case PASSWORD_TYPE_MD5:
 			if (!pg_md5_encrypt(client_pass,
 								role,
@@ -307,30 +322,33 @@ plain_crypt_verify(const char *role, const char *shadow_pass,
 				 */
 				return STATUS_ERROR;
 			}
-			client_pass = crypt_client_pass;
+			if (strcmp(crypt_client_pass, shadow_pass) == 0)
+				return STATUS_OK;
+			else
+			{
+				*logdetail = psprintf(_("Password does not match for user \"%s\"."),
+									  role);
+				return STATUS_ERROR;
+			}
 			break;
+
 		case PASSWORD_TYPE_PLAINTEXT:
+			if (strcmp(client_pass, shadow_pass) == 0)
+				return STATUS_OK;
+			else
+			{
+				*logdetail = psprintf(_("Password does not match for user \"%s\"."),
+									  role);
+				return STATUS_ERROR;
+			}
 			break;
-
-		default:
-
-			/*
-			 * This shouldn't happen. Plain "password" authentication should
-			 * be possible with any kind of stored password hash.
-			 */
-			*logdetail = psprintf(_("Password of user \"%s\" is in unrecognized format."),
-								  role);
-			return STATUS_ERROR;
 	}
 
-	if (strcmp(client_pass, shadow_pass) == 0)
-		retval = STATUS_OK;
-	else
-	{
-		*logdetail = psprintf(_("Password does not match for user \"%s\"."),
-							  role);
-		retval = STATUS_ERROR;
-	}
-
-	return retval;
+	/*
+	 * This shouldn't happen. Plain "password" authentication is possible
+	 * with any kind of stored password hash.
+	 */
+	*logdetail = psprintf(_("Password of user \"%s\" is in unrecognized format."),
+						  role);
+	return STATUS_ERROR;
 }
diff --git a/src/include/libpq/scram.h b/src/include/libpq/scram.h
index 78a52db684..fb21e056c8 100644
--- a/src/include/libpq/scram.h
+++ b/src/include/libpq/scram.h
@@ -31,5 +31,7 @@ extern char *scram_build_verifier(const char *username,
 					 const char *password,
 					 int iterations);
 extern bool is_scram_verifier(const char *verifier);
+extern bool scram_verify_plain_password(const char *username,
+							const char *password, const char *verifier);
 
 #endif   /* PG_SCRAM_H */
-- 
2.11.0

From 4439f773e10df07a1ccadc4acda97322eb456ead Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Tue, 14 Mar 2017 14:35:23 +0200
Subject: [PATCH 2/2] Add TAP tests for password-based authentication methods.

Tests all combinations of users with MD5, plaintext and SCRAM verifiers
stored in pg_authid, with 'plain', 'md5' and 'scram' authentication
methods.

Michael Paquier
---
 src/test/Makefile                         |  2 +-
 src/test/README                           |  3 ++
 src/test/authentication/Makefile          | 20 ++++++++
 src/test/authentication/README            | 16 ++++++
 src/test/authentication/t/001_password.pl | 84 +++++++++++++++++++++++++++++++
 5 files changed, 124 insertions(+), 1 deletion(-)
 create mode 100644 src/test/authentication/Makefile
 create mode 100644 src/test/authentication/README
 create mode 100644 src/test/authentication/t/001_password.pl

diff --git a/src/test/Makefile b/src/test/Makefile
index 3c2215849e..dbfa799a84 100644
--- a/src/test/Makefile
+++ b/src/test/Makefile
@@ -12,7 +12,7 @@ subdir = src/test
 top_builddir = ../..
 include $(top_builddir)/src/Makefile.global
 
-SUBDIRS = perl regress isolation modules recovery subscription
+SUBDIRS = perl regress isolation modules authentication recovery subscription
 
 # We don't build or execute examples/, locale/, or thread/ by default,
 # but we do want "make clean" etc to recurse into them.  Likewise for ssl/,
diff --git a/src/test/README b/src/test/README
index 0019782fb1..b5ccfc0cf6 100644
--- a/src/test/README
+++ b/src/test/README
@@ -8,6 +8,9 @@ individual contrib/ modules and in src/bin.
 Not all these tests get run by "make check". Check src/test/Makefile to see
 which tests get run automatically.
 
+authentication/
+  Tests for authentication
+
 examples/
   Demonstration programs for libpq that double as regression tests via
   "make check"
diff --git a/src/test/authentication/Makefile b/src/test/authentication/Makefile
new file mode 100644
index 0000000000..21ad15bea9
--- /dev/null
+++ b/src/test/authentication/Makefile
@@ -0,0 +1,20 @@
+#-------------------------------------------------------------------------
+#
+# Makefile for src/test/authentication
+#
+# Portions Copyright (c) 1996-2017, PostgreSQL Global Development Group
+# Portions Copyright (c) 1994, Regents of the University of California
+#
+# src/test/authentication/Makefile
+#
+#-------------------------------------------------------------------------
+
+subdir = src/test/authentication
+top_builddir = ../../..
+include $(top_builddir)/src/Makefile.global
+
+check:
+	$(prove_check)
+
+clean distclean maintainer-clean:
+	rm -rf tmp_check
diff --git a/src/test/authentication/README b/src/test/authentication/README
new file mode 100644
index 0000000000..5cffc7dc49
--- /dev/null
+++ b/src/test/authentication/README
@@ -0,0 +1,16 @@
+src/test/authentication/README
+
+Regression tests for authentication
+===================================
+
+This directory contains a test suite for authentication. SSL certificate
+authentication tests are kept separate, in src/test/ssl/, because they
+are more complicated, and are not safe to run in a multi-user system.
+
+
+Running the tests
+=================
+
+    make check
+
+NOTE: This requires the --enable-tap-tests argument to configure.
diff --git a/src/test/authentication/t/001_password.pl b/src/test/authentication/t/001_password.pl
new file mode 100644
index 0000000000..8726a23e0d
--- /dev/null
+++ b/src/test/authentication/t/001_password.pl
@@ -0,0 +1,84 @@
+# Set of tests for authentication and pg_hba.conf. The following password
+# methods are checked through this test:
+# - Plain
+# - MD5-encrypted
+# - SCRAM-encrypted
+# This test cannot run on Windows as Postgres cannot be set up with Unix
+# sockets and needs to go through SSPI.
+
+use strict;
+use warnings;
+use PostgresNode;
+use TestLib;
+use Test::More tests => 12;
+
+# Delete pg_hba.conf from the given node, add a new entry to it
+# and then execute a reload to refresh it.
+sub reset_pg_hba
+{
+	my $node = shift;
+	my $hba_method = shift;
+
+	unlink($node->data_dir . '/pg_hba.conf');
+	$node->append_conf('pg_hba.conf', "local all all $hba_method");
+	$node->reload;
+}
+
+# Test access for a single role, useful to wrap all tests into one.
+sub test_role
+{
+	my $node = shift;
+	my $role = shift;
+	my $method = shift;
+	my $expected_res = shift;
+	my $status_string = 'failed';
+
+	$status_string = 'success' if ($expected_res eq 0);
+
+	my $res = $node->psql('postgres', 'SELECT 1', extra_params => ['-U', $role]);
+	is($res, $expected_res,
+	   "authentication $status_string for method $method, role $role");
+}
+
+SKIP:
+{
+	skip "authentication tests cannot run on Windows", 12 if ($windows_os);
+
+	# Initialize master node
+	my $node = get_new_node('master');
+	$node->init;
+	$node->start;
+
+	# Create 3 roles with different password methods for each one. The same
+	# password is used for all of them.
+	$node->safe_psql('postgres', "SET password_encryption='scram'; CREATE ROLE scram_role LOGIN PASSWORD 'pass';");
+	$node->safe_psql('postgres', "SET password_encryption='md5'; CREATE ROLE md5_role LOGIN PASSWORD 'pass';");
+	$node->safe_psql('postgres', "SET password_encryption='plain'; CREATE ROLE plain_role LOGIN PASSWORD 'pass';");
+	$ENV{"PGPASSWORD"} = 'pass';
+
+	# For "trust" method, all users should be able to connect.
+	reset_pg_hba($node, 'trust');
+	test_role($node, 'scram_role', 'trust', 0);
+	test_role($node, 'md5_role', 'trust', 0);
+	test_role($node, 'plain_role', 'trust', 0);
+
+	# For plain "password" method, all users should also be able to connect.
+	reset_pg_hba($node, 'password');
+	test_role($node, 'scram_role', 'password', 0);
+	test_role($node, 'md5_role', 'password', 0);
+	test_role($node, 'plain_role', 'password', 0);
+
+	# For "scram" method, user "plain_role" and "scram_role" should be able to
+	# connect.
+	reset_pg_hba($node, 'scram');
+	test_role($node, 'scram_role', 'scram', 0);
+	test_role($node, 'md5_role', 'scram', 2);
+	test_role($node, 'plain_role', 'scram', 0);
+
+	# For "md5" method, users "plain_role" and "md5_role" should be able to
+	# connect.
+	reset_pg_hba($node, 'md5');
+	test_role($node, 'scram_role', 'md5', 2);
+	test_role($node, 'md5_role', 'md5', 0);
+	test_role($node, 'plain_role', 'md5', 0);
+}
-- 
2.11.0