Skip to content

Commit

Permalink
[keyserver] Update existing cookie password hashes to sha256
Browse files Browse the repository at this point in the history
Summary:
We are already using `sha256` hashes for new log-ins on `master`. This diff makes us convert existing log-ins (existing row in the MariaDB `cookies` table) to `sha256` hashes.

We can't recalculate the hash without the "cookie password", so we'll do this when the user sends a request in `updateCookie`.

It's safe to call `Viewer.cookieHash` and `Viewer.cookiePassword` in `updateCookie`, since `updateCookie` is only called for real, authenticated requests (HTML website, JSON responder, or WebSocket). `updateCookie` does not appear to be called with a bot or script viewer anywhere in the codebase.

The cookie itself (the one we send to the client) doesn't need to change, since it doesn't include the hash.

Test Plan:
Was tested in combination with preceding diff:

1. Log out on the web app
2. Check out a version of `master` with `397b4542fa7b38d8468038d74f3de84969f9dc36` and `12d02949bb4bb44a129163def1a7a056a7791b74` reverted
3. Log back in on the web app
4. Confirm that I have a `bcrypt` hash by running `SELECT * FROM cookies ORDER BY last_used DESC LIMIT 1` in MariaDB console
5. Close web app tab, kill `keyserver`, and check out `master` + D9563 + this diff
6. Open web app tab again
7. Confirm that I have a `sha256` hash by rerunning query from step 4
8. Confirm that the cookie ID is the same between steps 4 and 7

Reviewers: atul, tomek, inka

Reviewed By: atul

Subscribers: wyilio

Differential Revision: https://phab.comm.dev/D9564
  • Loading branch information
Ashoat committed Oct 22, 2023
1 parent f8605b0 commit d7d5043
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 5 deletions.
8 changes: 6 additions & 2 deletions keyserver/src/session/cookie-hash.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,20 @@
import crypto from 'crypto';
import bcrypt from 'twin-bcrypt';

function isBcryptHash(cookieHash: string): boolean {
return cookieHash.startsWith('$2y$');
}

function getCookieHash(cookiePassword: string): string {
return crypto.createHash('sha256').update(cookiePassword).digest('hex');
}

function verifyCookieHash(cookiePassword: string, cookieHash: string): boolean {
if (cookieHash.startsWith('$2y$')) {
if (isBcryptHash(cookieHash)) {
return bcrypt.compareSync(cookiePassword, cookieHash);
}
const expectedCookieHash = getCookieHash(cookiePassword);
return cookieHash === expectedCookieHash;
}

export { getCookieHash, verifyCookieHash };
export { isBcryptHash, getCookieHash, verifyCookieHash };
16 changes: 13 additions & 3 deletions keyserver/src/session/cookies.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,11 @@ import type { UserInfo } from 'lib/types/user-types.js';
import { values } from 'lib/utils/objects.js';
import { promiseAll } from 'lib/utils/promises.js';

import { getCookieHash, verifyCookieHash } from './cookie-hash.js';
import {
isBcryptHash,
getCookieHash,
verifyCookieHash,
} from './cookie-hash.js';
import { Viewer } from './viewer.js';
import type { AnonymousViewerData, UserViewerData } from './viewer.js';
import createIDs from '../creators/id-creator.js';
Expand Down Expand Up @@ -733,9 +737,15 @@ async function setNewSession(

async function updateCookie(viewer: Viewer) {
const time = Date.now();
const { cookieID } = viewer;
const { cookieID, cookieHash, cookiePassword } = viewer;

const updateObj = {};
updateObj.last_used = time;
if (isBcryptHash(cookieHash)) {
updateObj.hash = getCookieHash(cookiePassword);
}
const query = SQL`
UPDATE cookies SET last_used = ${time} WHERE id = ${cookieID}
UPDATE cookies SET ${updateObj} WHERE id = ${cookieID}
`;
await dbQuery(query);
}
Expand Down

0 comments on commit d7d5043

Please sign in to comment.