From fb296832f0c6ce44d7581533031fbd8a8e0b5e1a Mon Sep 17 00:00:00 2001 From: Bill Erickson Date: Wed, 22 Jul 2015 10:32:55 -0400 Subject: [PATCH] LP#1468422 More comments, tests are now pgtap-driven. Signed-off-by: Bill Erickson --- .../Pg/upgrade/XXXX.schema.password-storage.sql | 156 +++++++++++++-------- 1 file changed, 100 insertions(+), 56 deletions(-) diff --git a/Open-ILS/src/sql/Pg/upgrade/XXXX.schema.password-storage.sql b/Open-ILS/src/sql/Pg/upgrade/XXXX.schema.password-storage.sql index 844fa59391..0d4129b661 100644 --- a/Open-ILS/src/sql/Pg/upgrade/XXXX.schema.password-storage.sql +++ b/Open-ILS/src/sql/Pg/upgrade/XXXX.schema.password-storage.sql @@ -8,11 +8,11 @@ CREATE TABLE actor.passwd_type ( name TEXT UNIQUE NOT NULL, login BOOLEAN NOT NULL DEFAULT FALSE, regex TEXT, -- pending - crypt_algo TEXT, -- pending - -- gen_salt iter count; used with each new salt. - -- existing salts encode their iter count directly. - -- A value for iter_count tells us the password is - -- salted and it's our job to generate/manage the salt. + crypt_algo TEXT, -- e.g. 'bf' + + -- gen_salt() iter count used with each new salt. + -- A non-NULL value for iter_count is our indication the + -- password is salted and encrypted via crypt() iter_count INTEGER CHECK (iter_count IS NULL OR iter_count > 0) ); @@ -20,7 +20,7 @@ CREATE TABLE actor.passwd ( id SERIAL PRIMARY KEY, usr INTEGER NOT NULL REFERENCES actor.usr(id) ON DELETE CASCADE DEFERRABLE INITIALLY DEFERRED, - salt TEXT, + salt TEXT, -- will be NULL for non-crypt'ed passwords passwd TEXT NOT NULL, passwd_type TEXT NOT NULL REFERENCES actor.passwd_type(code) ON DELETE CASCADE DEFERRABLE INITIALLY DEFERRED, @@ -29,12 +29,15 @@ CREATE TABLE actor.passwd ( CONSTRAINT passwd_type_once_per_user UNIQUE (usr, passwd_type) ); --- Returns a new salt based on the current passwd_type encryption settings CREATE OR REPLACE FUNCTION actor.create_salt(pw_type TEXT) RETURNS TEXT AS $$ DECLARE type_row actor.passwd_type%ROWTYPE; BEGIN + /* Returns a new salt based on the passwd_type encryption settings. + * Returns NULL If the password type is not crypt()'ed. + */ + SELECT INTO type_row * FROM actor.passwd_type WHERE code = pw_type; IF NOT FOUND THEN @@ -51,8 +54,15 @@ END; $$ LANGUAGE PLPGSQL; --- Sets the password value, creating the password row if needed. --- new_pass is MD5(MD5(real-password) || salt) for 'main' passwords. +/* TODO: when a user changes their password in the application, the + app layer has access to the bare password. At that point, we + have the opportunity to store the new password without the MD5(MD5()) + intermediate hashing. Do we care? We would need a way to indicate + which passwords have the legacy intermediate hashing and which don't. + In either event, with the exception of migrate_passwd(), the DB + functions know or care nothing about intermediate hashing. Every + password is just a value that may or may not be internally crypt'ed. */ + CREATE OR REPLACE FUNCTION actor.set_passwd( pw_usr INTEGER, pw_type TEXT, new_pass TEXT, new_salt TEXT DEFAULT NULL) RETURNS BOOLEAN AS $$ @@ -60,6 +70,12 @@ DECLARE pw_salt TEXT; pw_text TEXT; BEGIN + /* Sets the password value, creating a new actor.passwd row if needed. + * If the password type supports it, the new_pass value is crypt()'ed. + * For crypt'ed passwords, the salt comes from one of 3 places in order: + * new_salt (if present), existing salt (if present), newly created + * salt. + */ IF new_salt IS NOT NULL THEN pw_salt := new_salt; @@ -67,9 +83,9 @@ BEGIN pw_salt := actor.get_salt(pw_usr, pw_type); IF pw_salt IS NULL THEN - -- We have no salt for this user + type. - -- Assume they want a new salt. If this type - -- is unsalted, create_salt() will return NULL. + /* We have no salt for this user + type. Assume they want a + * new salt. If this type is unsalted, create_salt() will + * return NULL. */ pw_salt := actor.create_salt(pw_type); END IF; END IF; @@ -94,16 +110,17 @@ BEGIN END; $$ LANGUAGE PLPGSQL; --- actor.passwd is not IDL-accessible. This function gives the --- application access to the salt. If the password type of "main" --- is requested and no password exists in actor.passwd, the user's --- existing password is migrated and the new salt is returned. CREATE OR REPLACE FUNCTION actor.get_salt(pw_usr INTEGER, pw_type TEXT) RETURNS TEXT AS $$ DECLARE pw_salt TEXT; type_row actor.passwd_type%ROWTYPE; BEGIN + /* Returns the salt for the requested user + type. If the password + * type of "main" is requested and no password exists in actor.passwd, + * the user's existing password is migrated and the new salt is returned. + * Returns NULL if the password type is not crypt'ed (iter_count is NULL). + */ SELECT INTO pw_salt salt FROM actor.passwd WHERE usr = pw_usr AND passwd_type = pw_type; @@ -123,14 +140,16 @@ BEGIN END; $$ LANGUAGE PLPGSQL; --- Migrates actor.usr.passwd to actor.passwd with --- password type 'main' and returns the salt. CREATE OR REPLACE FUNCTION actor.migrate_passwd(pw_usr INTEGER) RETURNS TEXT AS $$ DECLARE pw_salt TEXT; usr_row actor.usr%ROWTYPE; BEGIN + /* Migrates legacy actor.usr.passwd value to actor.passwd with + * a password type 'main' and returns the new salt. + */ + SELECT INTO usr_row * FROM actor.usr WHERE id = pw_usr; pw_salt := actor.create_salt('main'); @@ -145,15 +164,17 @@ BEGIN END; $$ LANGUAGE PLPGSQL; --- Returns TRUE if the password provided matches the in-db password. --- If the password type is salted, we compare the output of CRYPT(). --- test_passwd is MD5(MD5(password) || salt) for 'main' passwords. CREATE OR REPLACE FUNCTION actor.verify_passwd(pw_usr INTEGER, pw_type TEXT, test_passwd TEXT) RETURNS BOOLEAN AS $$ DECLARE pw_salt TEXT; BEGIN + /* Returns TRUE if the password provided matches the in-db password. + * If the password type is salted, we compare the output of CRYPT(). + * NOTE: test_passwd is MD5(MD5(password) || salt) for legacy + * 'main' passwords. + */ SELECT INTO pw_salt salt FROM actor.passwd WHERE usr = pw_usr AND passwd_type = pw_type; @@ -190,51 +211,74 @@ INSERT INTO actor.passwd_type -- INLINE TESTS --------------- +-- TODO: move to new t/ file. + +\set ECHO none +\set QUIET 1 +-- Turn off echo and keep things quiet. + +-- Format the output for nice TAP. +\pset format unaligned +\pset tuples_only true +\pset pager --- concerto user br1mclark --- confirm the pre-migration password is what we think it is. -SELECT TRUE AS verify_old_pw - FROM actor.usr WHERE id = 187 AND passwd = MD5('montyc1234'); - --- This should result in a migration -SELECT actor.get_salt(187, 'main') AS new_salt; --- Or directly migrate --- SELECT actor.migrate_passwd(187); - --- see what the new password row looks like -SELECT * FROM actor.passwd WHERE usr = 187; - --- see if the new-style password verifies -SELECT actor.verify_passwd(187, 'main', - MD5( - MD5('montyc1234') || - actor.get_salt(187, 'main')) - ) AS verify_pw_good; - --- make sure a bad password fails -SELECT actor.verify_passwd(187, 'main', - MD5( - MD5('montyc1234XXX') || - actor.get_salt(187, 'main')) - ) AS verify_pw_bad; +-- Revert all changes on failure. +\set ON_ERROR_ROLLBACK 1 +\set ON_ERROR_STOP true +\set QUIET 1 +-- Plan the tests. +SELECT plan(6); + +SELECT ok( + (SELECT TRUE AS verify_old_pw FROM actor.usr + WHERE id = 187 AND passwd = MD5('montyc1234')), + 'Legacy password should match' +); + +SELECT isnt_empty( + 'SELECT actor.get_salt(187, ''main'')', + 'get_salt() returns a new salt' +); + +SELECT isnt_empty( + 'SELECT * FROM actor.passwd WHERE usr = 187 AND passwd_type = ''main''', + 'get_salt() should migrate the password' +); + +SELECT ok( + (SELECT actor.verify_passwd(187, 'main', + MD5(MD5('montyc1234') || actor.get_salt(187, 'main')))), + 'verify_passwd should verify migrated password' +); + +SELECT ok( + (SELECT NOT ( + SELECT actor.verify_passwd(187, 'main', + MD5(MD5('BADPASSWORD') || actor.get_salt(187, 'main')))) + ), + 'verify_passwd should fail with wrong password' +); + +-- This code chunk mimics the application changing a user's password DO $$ DECLARE new_salt TEXT; - DECLARE ok BOOLEAN; BEGIN - -- modify password and use a new salt - + -- we have to capture the salt, because subsequent + -- calls will create a new one. SELECT INTO new_salt actor.create_salt('main'); - PERFORM actor.set_passwd( 187, 'main', MD5(MD5('bobblehead') || new_salt), new_salt); - - SELECT INTO ok actor.verify_passwd( - 187, 'main', MD5(MD5('bobblehead') || actor.get_salt(187, 'main'))); - - RAISE NOTICE 'new password check resulted in %', ok; END $$; +SELECT ok( + (SELECT actor.verify_passwd(187, 'main', + MD5(MD5('bobblehead') || actor.get_salt(187, 'main')))), + 'verify_passwd should verify new password' +); + +-- Finish the tests and clean up. +SELECT * FROM finish(); ROLLBACK; --COMMIT; -- 2.11.0